-
Notifications
You must be signed in to change notification settings - Fork 12
Perpetual tranche v1.1.0 updates #126
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
Conversation
| } else if (!authorize && _authorizedRollers.contains(roller)) { | ||
| _authorizedRollers.remove(roller); | ||
| } else { | ||
| revert RollerAuthorizationFailed(); |
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.
Any reason to make this not idempotent?
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.
Torn between both options. Reverting intuitively feels safer ..
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.
Depends on context, I guess.
Reverting can make interoperability between smart contracts more difficult, but it can make calls from outside like Gelato potentially easier. I'd normally default to not reverting, just like you default to not throwing exception in the JVM.
In this case, it shouldn't /really/ matter since this'll generally be called by governance. It just seems a bit weird to revert the authorize call when, at the end of it, the address is actually still authorized. I think that would still be successful as far as the caller is concerned--it just happens to be a no-op.
I can see some weird cases where a governance action might want to pre-emptively call an authorize function as a pre-step before another op, just to make sure the starting context is correct and not rely on a separate governance action to execute first. So I think I'd still prefer idempotency.
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.
👍
Co-authored-by: Brandon Iles <brandon@fragments.org>
d5888de to
c2403e1
Compare
c2403e1 to
7513f34
Compare
| } else if (!authorize && _authorizedRollers.contains(roller)) { | ||
| _authorizedRollers.remove(roller); | ||
| } else { | ||
| revert RollerAuthorizationFailed(); |
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.
Depends on context, I guess.
Reverting can make interoperability between smart contracts more difficult, but it can make calls from outside like Gelato potentially easier. I'd normally default to not reverting, just like you default to not throwing exception in the JVM.
In this case, it shouldn't /really/ matter since this'll generally be called by governance. It just seems a bit weird to revert the authorize call when, at the end of it, the address is actually still authorized. I think that would still be successful as far as the caller is concerned--it just happens to be a no-op.
I can see some weird cases where a governance action might want to pre-emptively call an authorize function as a pre-step before another op, just to make sure the starting context is correct and not rely on a separate governance action to execute first. So I think I'd still prefer idempotency.
brandoniles
left a comment
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.
LGTM, just the last minor comment about emitting the event
2609265 to
e2979d2
Compare
e2979d2 to
8ba96c7
Compare
depositBondreference from storageaddress(0)to allow for eventual retirementgetReserveTrancheValue) to perp which calculates the value of each reserve token.onlyOwnermethods and events out of the interface definition