-
Notifications
You must be signed in to change notification settings - Fork 92
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
BUIDL GSM Integration #421
base: main
Are you sure you want to change the base?
Conversation
|
…enforce no remaining balance in converter
test: remaining gho balance
…llAssetWithSig test
test: add assertions for testSellAssetWithSig
test: clean up referenced amounts in tests
IGhoToken(GHO_TOKEN).mint(address(this), grossAmount); | ||
IGhoToken(GHO_TOKEN).transfer(receiver, ghoBought); | ||
// TRIGGER ERROR: invalid transfer of GHO amount to GSM Converter (msg.sender) | ||
IGhoToken(GHO_TOKEN).transfer(msg.sender, 1); |
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.
Similarly here, you can just have this one broken function that you call in your test. Can't use vm.mockCall() because it's actually transferring tokens in this case.
I still need to look into it, but you could perhaps just fuzz test these functions to test if calculations would fail instead of using this mock pattern, because it's effectively testing a contract out of scope of our repo, no?
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.
you can just have this one broken function that you call in your test
This one I think was harder to simulate bc it needed to happen within the flow of the converter, rather than within the test.
effectively testing a contract out of scope of our repo, no
I don't think it is out of scope of the repo, but perhaps out of scope of the converter because it is the GSM implementation. Do you think from the POV of the converter I can assume that the GSM will function properly and leave the require statement coverage uncovered that checks the correct amount of tokens are returned from GSM?
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 think you can leave this mock in here, but can't you reduce the code? I think you probably left in some functions that are unused in the tests involving this mock
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.
yes I think so, let me work on that
closes #420