-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚡️ Zap #38
⚡️ Zap #38
Conversation
@@ -168,7 +180,7 @@ contract Engine is | |||
uint128 _accountId, | |||
uint256 _wordPos, | |||
uint256 _mask | |||
) external override { | |||
) external payable override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I presume we have made this and other functions payable so they can be used in MulticallablePayable
alongside a call to fulfillOracleQuery
? The only issue I could see related to this is a future dev thinking - oh there are loads of payable functions, I can add one. But that is not a real security issue now. Can you think of any other issues that could arise from having these functions payable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this quite a bit, and since msg.value
is never used within any of these functions, there is no protocol risk I am aware of by making them payable. (users can send funds to the contract accidentally but tbf that could also be done before via self-destruct).
As a side note, I have read/heard that the payable
function modifier might be removed in future solidity versions, and every function will be payable. So that makes me feel better about it.
Nice work! All looks good to me. Just a couple of comments, nothing significant. Probably my main question is around the payable functions. |
Add Zap library and implement hooks
Description
Related issue(s)
Closes #37
Motivation and Context
SNX v3 Andromeda supports $USDC collateral wrapping