Test token hooks #1198
Test token hooks #1198
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1198 +/- ##
==========================================
- Coverage 91.57% 91.56% -0.02%
==========================================
Files 47 47
Lines 1223 1233 +10
==========================================
+ Hits 1120 1129 +9
- Misses 103 104 +1 see 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
| fn assert_event_before_update( | ||
| ref self: EventSpy, | ||
| contract: ContractAddress, | ||
| from: ContractAddress, | ||
| recipient: ContractAddress, | ||
| amount: u256 | ||
| ) { | ||
| let expected = ERC20MockWithHooks::Event::BeforeUpdate( | ||
| ERC20MockWithHooks::BeforeUpdate { from, recipient, amount } | ||
| ); | ||
| self.assert_emitted_single(contract, expected); | ||
| } | ||
|
|
||
| fn assert_event_after_update( | ||
| ref self: EventSpy, | ||
| contract: ContractAddress, | ||
| from: ContractAddress, | ||
| recipient: ContractAddress, | ||
| amount: u256 | ||
| ) { | ||
| let expected = ERC20MockWithHooks::Event::AfterUpdate( |
There was a problem hiding this comment.
What do you think about adding assert_no_events_left_from(contract) to these?
There was a problem hiding this comment.
If you agree, I'd apply this to erc721 as well
There was a problem hiding this comment.
update itself emits a Transfer event, so I think this would only make sense on the after hook. But not sure how valuable it is?
I'll prioritize the other comments + adding the tests to ERC1155 and then see if I can add this
There was a problem hiding this comment.
update itself emits a Transfer event, so I think this would only make sense on the after hook
To be comprehensive, my thinking is to test that only the target hook is executed and only once. I suppose you can argue that we know it works as expected through the myriad other tests we have...your call if you think it's worth it
There was a problem hiding this comment.
Since we are getting the release today I'll go ahead and merge. Will try to add this to a next PR. Thanks!
|
Comments have been incorporated + renamed the new mocks to |
Fixes #1062
PR Checklist