Feature/better event assertions #83
Conversation
| emit FeeAccrued(WETH_A, ALICE, expectedFeeAccruedAmount); | ||
|
|
||
| vm.expectEmit(true, true, true, true); | ||
| emit OptionsWritten(testOptionId, ALICE, claimId, 1); |
There was a problem hiding this comment.
@0xAlcibiades @neodaoist what do we think makes the most sense here to emit. Currently the index of the claim is emitted (1, 2 ,3 ,4). Do we want to emit the entire claim ID instead with the encoded option ID in the top 160b?
There was a problem hiding this comment.
(Not related to changes in this PR btw)
There was a problem hiding this comment.
Honestly I would love to talk through how dapp devs or end users might want to view and monitor this event. Need more context to have an opinion yet
src/test/OptionSettlement.t.sol
Outdated
| @@ -543,11 +570,16 @@ contract OptionSettlementTest is Test, NFTreceiver { | |||
| uint96(expectedUnderlyingAmount) | |||
| ); | |||
|
|
|||
| engine.claim(claimId); | |||
| // engine.claim(claimId); | |||
There was a problem hiding this comment.
Was the test passing before this? Should we add or update an assert here?
There was a problem hiding this comment.
Ah woops, I meant to remove this altogether. There's no need to call the view function, the test exercises the events emitted from redeem().
|
Yeah, I’d think so.
Sent from Proton Mail for iOS
…On Thu, Oct 27, 2022 at 7:23 PM, Flip-Liquid ***@***.***> wrote:
@Flip-Liquid commented on this pull request.
---------------------------------------------------------------
In [src/test/OptionSettlement.t.sol](#83 (comment)):
>
vm.prank(ALICE);
engine.write(testOptionId, 1);
}
+ function testEventWriteWhenExistingClaim() public {
+ uint256 expectedFeeAccruedAmount = ((testUnderlyingAmount / 10_000) * engine.feeBps());
+
+ vm.prank(ALICE);
+ uint256 claimId = engine.write(testOptionId, 1);
+
+ vm.expectEmit(true, true, true, true);
+ emit FeeAccrued(WETH_A, ALICE, expectedFeeAccruedAmount);
+
+ vm.expectEmit(true, true, true, true);
+ emit OptionsWritten(testOptionId, ALICE, claimId, 1);
***@***.***(https://github.com/0xAlcibiades) ***@***.***(https://github.com/neodaoist) what do we think makes the most sense here to emit. Currently the index of the claim is emitted (1, 2 ,3 ,4). Do we want to emit the entire claim ID instead with the encoded option ID in the top 160b?
—
Reply to this email directly, [view it on GitHub](#83 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AVOT3C6Z56HSKCR6OPSNER3WFMFI3ANCNFSM6AAAAAARQO6KII).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Resolves #74
The snapshot diff is way nastier than my changes, think it includes some other recent changes, just heads up. Indexing claimId on the event should only cost 110 gas more on write() calls.