-
Notifications
You must be signed in to change notification settings - Fork 50
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tests): add GaugeReward unit tests #272
feat(tests): add GaugeReward unit tests #272
Conversation
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 great! Just some quibbles on tests
test/TokenVault.test.ts
Outdated
await token.mock.approve.withArgs(wallet2.address, '1111').returns(true) | ||
await vault.increaseERC20Allowance(token.address, wallet2.address, '1111') | ||
await tokenVault.increaseERC20Allowance(token.address, wallet2.address, '1111') |
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 know what is interesting about this test- is that it doesn't assert the state change.
The mock doesn't have an expectation, so if the mock isn't called this test will still pass.
In this case, the easiest thing would be to instantiate a real token and check to see that the allowance was changed properly
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.
Done: b945745
test/TokenVault.test.ts
Outdated
await token.mock.approve.withArgs(wallet2.address, '1111').returns(true) | ||
await vault.connect(wallet3).increaseERC20Allowance(token.address, wallet2.address, '1111') | ||
await tokenVault.connect(wallet3).increaseERC20Allowance(token.address, wallet2.address, '1111') |
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.
Likewise 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.
Done: b945745
test/TokenVault.test.ts
Outdated
await token.mock.approve.withArgs(wallet2.address, '111').returns(true) | ||
await vault.decreaseERC20Allowance(token.address, wallet2.address, '1000') | ||
await tokenVault.decreaseERC20Allowance(token.address, wallet2.address, '1000') |
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.
and 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.
Done: b945745
test/TokenVault.test.ts
Outdated
await token.mock.approve.withArgs(wallet2.address, '0').returns(true) | ||
await vault.connect(wallet3).decreaseERC20Allowance(token.address, wallet2.address, '1111') | ||
await tokenVault.connect(wallet3).decreaseERC20Allowance(token.address, wallet2.address, '1111') |
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.
and 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.
Done: b945745
test/TokenVault.test.ts
Outdated
await token.mock.approve.withArgs(wallet2.address, '0').returns(true) | ||
await vault.connect(wallet3).decreaseERC20Allowance(token.address, wallet2.address, '1111') | ||
await tokenVault.connect(wallet3).decreaseERC20Allowance(token.address, wallet2.address, '1111') | ||
}) |
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.
how about some revert checks for non-owner/managers?
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.
Done: b945745
5a61ae4
to
38cb693
Compare
38cb693
to
181e3c7
Compare
b945745
to
4e1d4ef
Compare
No description provided.