feat: modify batcher contract to support erc20 sendMany #192

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

gianchandania

Please add tests for the method

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

contracts/Batcher.sol

+    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

contracts/Batcher.sol

+  ) 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

contracts/Batcher.sol

+  ) 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

contracts/Batcher.sol

+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

contracts/Batcher.sol

test/batcher.js

Ticket: COIN-2782

sachushaji

Good Work!!

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

contracts/Batcher.sol

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


NIT: comments on errros

sachushaji · 2025-01-31T09:03:46Z

test/batcher.js

+        await checkBalance(sender, 500);
+        await checkBalance(recipients[0], 0);
+        await checkBalance(recipients[1], 0);


NIT: you can use promise.all

will make changes here.
https://bitgoinc.atlassian.net/browse/COIN-3005

mullapudipruthvik

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

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

contracts/Batcher.sol

+
+    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, sachushaji and mullapudipruthvik 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 had a problem deploying to dev January 28, 2025 10:48 — with GitHub Actions Failure

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

kamleshmugdiya had a problem deploying to dev January 28, 2025 10:49 — with GitHub Actions Failure

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

kamleshmugdiya had a problem deploying to dev January 29, 2025 06:24 — with GitHub Actions Failure

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

kamleshmugdiya had a problem deploying to dev January 29, 2025 06:25 — with GitHub Actions Failure

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 had a problem deploying to dev January 30, 2025 07:22 — with GitHub Actions Failure

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 had a problem deploying to dev January 30, 2025 10:25 — with GitHub Actions Failure

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

View reviewed changes

test/batcher.js Fixed Show fixed Hide fixed

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

kamleshmugdiya had a problem deploying to dev January 30, 2025 10:37 — with GitHub Actions Failure

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
4 checks passed

kamleshmugdiya deleted the COIN-2782 branch February 4, 2025 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modify batcher contract to support erc20 sendMany #192

feat: modify batcher contract to support erc20 sendMany #192

kamleshmugdiya commented Jan 21, 2025

gianchandania left a comment

gianchandania Jan 22, 2025

gianchandania Jan 22, 2025

kamleshmugdiya Jan 29, 2025

gianchandania Jan 22, 2025

kamleshmugdiya Jan 23, 2025

kamleshmugdiya Jan 29, 2025

sachushaji Jan 28, 2025

sachushaji Jan 28, 2025

kamleshmugdiya Jan 29, 2025

kamleshmugdiya commented Jan 29, 2025

sachushaji Jan 30, 2025

kamleshmugdiya Jan 30, 2025

sachushaji Jan 31, 2025

sachushaji left a comment

sachushaji Jan 31, 2025

sachushaji Jan 31, 2025

kamleshmugdiya Feb 4, 2025

mullapudipruthvik left a comment

mullapudipruthvik Feb 3, 2025

kamleshmugdiya Feb 4, 2025

kamleshmugdiya commented Feb 3, 2025

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

feat: modify batcher contract to support erc20 sendMany #192

feat: modify batcher contract to support erc20 sendMany #192

Conversation

kamleshmugdiya commented Jan 21, 2025

gianchandania left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamleshmugdiya commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachushaji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullapudipruthvik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamleshmugdiya commented Feb 3, 2025