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
Zero distribution #878
Zero distribution #878
Conversation
emitted: true, | ||
}, | ||
{ | ||
contract: token1, |
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 dont think this should fire?
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.
Agreed, it has emitted: false
to check for that.
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.
We dont need the args
param if we are checking for emitted = false, maybe thats the confusion.
args: [rTokenTrader.address, backingManager.address, issueAmount.add(1)], | ||
emitted: true, | ||
}, | ||
{ |
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 don't think this should fire?
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.
Looks good overall. Left some comments.
emitted: true, | ||
}, | ||
{ | ||
contract: token1, |
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.
We dont need the args
param if we are checking for emitted = false, maybe thats the confusion.
} else if (address(tokenToBuy) == address(main.rToken())) { | ||
require(revTotals.rTokenTotal == 0, "rTokenTotal > 0"); | ||
} else { | ||
revert("invalid tokenToBuy"); |
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.
This is not going to be covered with our default tests. So we can either deploy a standalone mock case with a RevTrader initialized to another tokenToBuy
(not generally what we do in most tests as it would not be our "interface") or we add the untestable
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.
Adding the comment
Resolves code-423n4/2023-06-reserve-findings#34