test: add tests for required finders fee bps and ensuring a valid update on newly set offers #132
Conversation
| @@ -110,6 +110,11 @@ contract OffersV1Test is DSTest { | |||
| offers.createOffer(address(token), 0, address(0), 1 ether, 1000); | |||
| } | |||
|
|
|||
| function testFail_CannotCreateOfferWithInvalidFindersFeeBps() public { | |||
| vm.prank(address(maker)); | |||
| offers.createOffer(address(token), 0, address(0), 1 ether, 10001); | |||
There was a problem hiding this comment.
10001 bps (basis points) is just one higher than what the require() looks for:
v3/contracts/modules/Offers/V1/OffersV1.sol
Line 159 in 60b2875
| @@ -200,6 +205,20 @@ contract OffersV1Test is DSTest { | |||
| vm.stopPrank(); | |||
| } | |||
|
|
|||
| function testRevert_CannotUpdateOfferWithPreviousAmount() public { | |||
There was a problem hiding this comment.
I'm open to renaming this (and the other) function name to match whatever naming convention the team prefers. ✌️
|
|
||
| vm.expectRevert("setOfferAmount invalid _amount"); | ||
|
|
||
| offers.setOfferAmount{value: 1 ether}(address(token), 0, 1, address(0), 1 ether); |
There was a problem hiding this comment.
The contract requires the newly set offer amount to be different from than previous:
v3/contracts/modules/Offers/V1/OffersV1.sol
Lines 234 to 235 in 60b2875
Since it's handled this way, it feels like a no-op in the same way we illustrate it for an ask already existing for a token. If that's the case, should we also reflect this behavior as such in the associated UML diagram?
|
hey @almndbtr thanks for adding these tests! they look great. |
|
Thanks again @kulkarohan ! 😌 |
Description
Towards #126
This adds missing tests for the Offers v1.0 contract.
Motivation and Context
These changes add to the work in #126 by including tests for required finders fee bps and ensuring a valid update on newly set offers. I noticed a handful of other tests were written to check for these cases (like checking that only the seller can update an offer or handling the case where an incoming transfer message value is less than the expected amount).
How has this been tested?
I ran
yarn testand they passed on my machine.Checklist: