feat: modify batcher contract to support erc20 sendMany by kamleshmugdiya · Pull Request #192 · BitGo/eth-multisig-v4

kamleshmugdiya · 2025-01-21T07:25:22Z

gianchandania

Please add tests for the method

gianchandania · 2025-01-22T14:10:38Z

+    address[] calldata recipients,
+    uint256[] calldata amounts
+  ) external lockCall {
+    require(recipients.length == amounts.length, "Length mismatch");


I think we should add other validations which are there in batch method here as well

require(recipients.length != 0, 'Must send to at least one person');
require(recipients.length < 256, 'Too many recipients');

should check what this number should be instead of 256

Also length mismatch error is bit ambiguous. let's make this error message same as batch method

kept the limit dynamic since it can be different for different chains.
added a function changeBatchTransferLimit to change the limit.

gianchandania · 2025-01-22T14:16:04Z

+  ) external lockCall {
+    require(recipients.length == amounts.length, "Length mismatch");
+    for (uint256 i = 0; i < recipients.length; i++) {
+      TransferHelper.safeTransferFrom(token, msg.sender, recipients[i], amounts[i]);


Ideally there should not be a case of partial success, either we send successfully to all the recipients, or none of the transfer should go through.

I tried to do partial success with giving less approval.
approval given was 20.
tried to send 30 here.
but it failed.

can you suggest some other way to try where partial success can be possible?

as suggested, I tried to send to 5 recipients while giving the gas limit for only 2.
the tx failed on chain.
https://holesky.etherscan.io/tx/0x42d64222124a57e5ee7135be7794fcf3d81df5563be3b55cb5a4df81803f2bf9

sachushaji · 2025-01-28T06:49:08Z

+  ) external lockCall {
+    require(recipients.length != 0, 'Must send to at least one person');
+    require(recipients.length == amounts.length, "Unequal recipients and values");
+    for (uint256 i = 0; i < recipients.length; i++) {


Suggested change

for (uint256 i = 0; i < recipients.length; i++) {

for (uint8 i = 0; i < recipients.length; i++) {

https://medium.com/coinmonks/mastering-solidity-require-and-custom-errors-in-ethereum-contracts-b491565f1592

You can also change the contract to use custom errors instead of require statements

used uint16.
also added custom errors.

kamleshmugdiya · 2025-01-29T06:29:13Z

Please add tests for the method

added few tests.

sachushaji · 2025-01-30T07:34:24Z

+error EmptyRecipientsList();
+error UnequalRecipientsAndValues();
+error TooManyRecipients(uint256 provided, uint256 limit);
+error TokenTransferFailed(address token, address from, address to, uint256 amount);


Add comments following the natspec format for everything

NIT: comments on errros

Ticket: COIN-2782

sachushaji

Good Work!!

sachushaji · 2025-01-31T09:00:14Z

+error EmptyRecipientsList();
+error UnequalRecipientsAndValues();
+error TooManyRecipients(uint256 provided, uint256 limit);
+error TokenTransferFailed(address token, address from, address to, uint256 amount);


NIT: comments on errros

mullapudipruthvik

LGTM
Have we figured out the owner and how do we interact with the contract?

mullapudipruthvik · 2025-02-03T05:56:29Z

+
+    IERC20 safeToken = IERC20(token);
+    for (uint16 i = 0; i < recipients.length; i++) {
+      safeToken.safeTransferFrom(msg.sender, recipients[i], amounts[i]);


should we allow 0 value transfers ?

will handle it here.
https://bitgoinc.atlassian.net/browse/COIN-3005

what is the expected behaviour?

should it skip the 0 value transfers and continue with other transfers?

fail entirely (keeping it atomic).

kamleshmugdiya · 2025-02-03T09:56:17Z

LGTM Have we figured out the owner and how do we interact with the contract?

Not sure about the owner.
I am thinking we will need a script and a workflow (since the owner will be bitgo key) to modify the limit.

kamleshmugdiya temporarily deployed to dev January 21, 2025 07:25 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from f4779d5 to 7a1176e Compare January 21, 2025 07:28

kamleshmugdiya temporarily deployed to dev January 21, 2025 07:28 — with GitHub Actions Inactive

kamleshmugdiya requested review from gianchandania, mullapudipruthvik and sachushaji January 21, 2025 07:56

gianchandania requested changes Jan 22, 2025

View reviewed changes

kamleshmugdiya marked this pull request as draft January 23, 2025 05:49

kamleshmugdiya force-pushed the COIN-2782 branch from 7a1176e to 54c9321 Compare January 23, 2025 17:51

kamleshmugdiya temporarily deployed to dev January 23, 2025 17:51 — with GitHub Actions Inactive

sachushaji reviewed Jan 28, 2025

View reviewed changes

kamleshmugdiya force-pushed the COIN-2782 branch from 54c9321 to 91304b5 Compare January 28, 2025 10:48

kamleshmugdiya temporarily deployed to dev January 28, 2025 10:48 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from 91304b5 to 85c7b1f Compare January 28, 2025 10:49

kamleshmugdiya temporarily deployed to dev January 28, 2025 10:49 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from 85c7b1f to 2506542 Compare January 29, 2025 06:24

kamleshmugdiya temporarily deployed to dev January 29, 2025 06:24 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from 2506542 to 5a2cf63 Compare January 29, 2025 06:25

kamleshmugdiya temporarily deployed to dev January 29, 2025 06:25 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from 5a2cf63 to 73ecc14 Compare January 29, 2025 06:33

kamleshmugdiya temporarily deployed to dev January 29, 2025 06:33 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from 73ecc14 to eca5b19 Compare January 29, 2025 06:39

kamleshmugdiya temporarily deployed to dev January 30, 2025 06:19 — with GitHub Actions Inactive

kamleshmugdiya force-pushed the COIN-2782 branch from e79e1f9 to 92bc37b Compare January 30, 2025 07:22

kamleshmugdiya temporarily deployed to dev January 30, 2025 07:22 — with GitHub Actions Inactive

kamleshmugdiya requested review from mullapudipruthvik January 30, 2025 07:45

kamleshmugdiya force-pushed the COIN-2782 branch from 92bc37b to 932dbb5 Compare January 30, 2025 07:46

kamleshmugdiya temporarily deployed to dev January 30, 2025 07:46 — with GitHub Actions Inactive

sachushaji reviewed Jan 30, 2025

View reviewed changes

kamleshmugdiya force-pushed the COIN-2782 branch from 932dbb5 to ae2cb27 Compare January 30, 2025 10:25

kamleshmugdiya temporarily deployed to dev January 30, 2025 10:25 — with GitHub Actions Inactive

github-advanced-security AI found potential problems Jan 30, 2025

View reviewed changes

Comment thread test/batcher.js Fixed

kamleshmugdiya force-pushed the COIN-2782 branch from ae2cb27 to aec9d8d Compare January 30, 2025 10:37

kamleshmugdiya temporarily deployed to dev January 30, 2025 10:37 — with GitHub Actions Inactive

feat: modify batcher contract to support erc20 sendMany

83e6911

Ticket: COIN-2782

kamleshmugdiya force-pushed the COIN-2782 branch from aec9d8d to 83e6911 Compare January 30, 2025 10:39

kamleshmugdiya temporarily deployed to dev January 30, 2025 10:39 — with GitHub Actions Inactive

kamleshmugdiya requested a review from sachushaji January 31, 2025 04:33

sachushaji approved these changes Jan 31, 2025

View reviewed changes

gianchandania approved these changes Feb 3, 2025

View reviewed changes

mullapudipruthvik approved these changes Feb 3, 2025

View reviewed changes

kamleshmugdiya merged commit a503f9f into master Feb 4, 2025

kamleshmugdiya mentioned this pull request Feb 14, 2025

chore: add test for token with blacklisted address #199

Merged

	for (uint256 i = 0; i < recipients.length; i++) {
	for (uint8 i = 0; i < recipients.length; i++) {

Uh oh!

Conversation

kamleshmugdiya commented Jan 21, 2025

Uh oh!

gianchandania left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamleshmugdiya commented Jan 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sachushaji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mullapudipruthvik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamleshmugdiya commented Feb 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants