feat: add batch function #1070
feat: add batch function #1070
Conversation
|
Thank you for the PR Andrei. I'd like to make a small suggestion. When a PR depends on another PR, can we please have the base branch set as the branch from another PR? This makes it easier to review and work with a child PR asynchronously. I know that by changing the URL I can see the diff but I use the "viewed checkbox" feature in Github UI to track which files have been reviewed and which are left to be reviewed. To be more explicit, In the description, it can be mentioned that this PR requires this another PR to be merged first. Does that make sense? |
andreivladbrg
force-pushed
the
feat/batch
branch
2 times, most recently
from
d8ed354 to
0e35fc1
Compare
| bytes[] memory calls = new bytes[](1); | ||
| calls[0] = abi.encodeCall(dai.balanceOf, (users.recipient)); | ||
|
|
||
| Batch(address(lockup)).batch(calls); |
There was a problem hiding this comment.
Missing expectRevert statement.
There was a problem hiding this comment.
hmm, curious why the test isn't failing then, i guess the branch should say it should do nothing
There was a problem hiding this comment.
Did you figure out why its not failing?
There was a problem hiding this comment.
It did not revert because dai.balanceOf encodes to balanceOf(address) selector which is a valid function in ERC721 contract. I fixed it in 5713f40.
I also want to mention that because the test was not failing, changing branch to it should do nothing should not be a solution. This is an example of white box testing where you tried to curve fit the test to made them pass. Referring to my discussion on white box testing vs black box testing, in a black box testing, we wont change the test but rather investigate the internal logic in such cases.
I look forward to see your comments on my discussion.
|
@smol-ninja just pushed a commit to addres your feedback, lmk if it looks good |
smol-ninja
force-pushed
the
feat/batch
branch
from
4865b3c to
104a048
Compare
test: add minimal tests for batch function
smol-ninja
force-pushed
the
feat/batch
branch
from
ba555e2 to
743e494
Compare
* feat: add batch contract test: add minimal tests for batch function * shub feedback * test: fix test for batch * fix: correct import of Batch contract --------- Co-authored-by: smol-ninja <[email protected]>
Closes #1053