-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: event withdrawn amounts #7
fix: event withdrawn amounts #7
Conversation
Signed-off-by: Pablo Maldonado <[email protected]>
UMA-2040 HoneyPotEmptied and PotReset not showing correct amounts
https://github.com/UMAprotocol/oev-demo/blob/master/src/HoneyPot.sol#L61 _emptyPotForUser is updating the stored balance before the events are emitted making the events to have 0 amounts. |
Signed-off-by: Pablo Maldonado <[email protected]>
src/HoneyPot.sol
Outdated
@@ -53,17 +53,17 @@ contract HoneyPot is Ownable { | |||
(, int256 currentPrice,,,) = oracle.latestRoundData(); | |||
require(currentPrice >= 0, "Invalid price from oracle"); | |||
|
|||
HoneyPotDetails storage userPot = honeyPots[honeyPotCreator]; | |||
HoneyPotDetails memory userPot = honeyPots[honeyPotCreator]; |
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.
Storage wasn't needed here.
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.
tiny nit: why change this?
In general, I think using storage references is usually (there are some exceptions) preferable (gas-wise) to copying the entire struct to memory because you may not actually need to SLOAD every element of the struct and you may end up using less memory overall by avoiding the copy. Does that make sense?
This is just a demo contract, so it's not super important, just wanted to make a note of it!
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!
src/HoneyPot.sol
Outdated
@@ -53,17 +53,17 @@ contract HoneyPot is Ownable { | |||
(, int256 currentPrice,,,) = oracle.latestRoundData(); | |||
require(currentPrice >= 0, "Invalid price from oracle"); | |||
|
|||
HoneyPotDetails storage userPot = honeyPots[honeyPotCreator]; | |||
HoneyPotDetails memory userPot = honeyPots[honeyPotCreator]; |
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.
tiny nit: why change this?
In general, I think using storage references is usually (there are some exceptions) preferable (gas-wise) to copying the entire struct to memory because you may not actually need to SLOAD every element of the struct and you may end up using less memory overall by avoiding the copy. Does that make sense?
This is just a demo contract, so it's not super important, just wanted to make a note of it!
Signed-off-by: Pablo Maldonado <[email protected]>
Changes proposed in this PR: