Question on the logic of this model

[usgeeus]

For sendERC20, if one of transfer fails, this whole transaction fails, But for sendETH, if any transfer fails, the transaction succeeds and refund the failedAmount to the msg.sender. Any motivation behind this? Why do I ask, because it's calculating the failedAmount, refunding it back, and using extra gas on this.

[bayram98]

For sendERC20, if one of transfer fails, this whole transaction fails, But for sendETH, if any transfer fails, the transaction succeeds and refund the failedAmount to the msg.sender. Any motivation behind this? Why do I ask, because it's calculating the failedAmount, refunding it back, and using extra gas on this.

The logic is, I wanted to be more user friendly by checking if the is the failed ETH or not on SendETH method. Yes, the checks are more gas costly but I though it might prevent more failure cases.

I think there is two way I can do. Which one do you think would be better?

1. Keep as it.
2. Remove the remain calculations in sendETH

[usgeeus]

I prefer option 2, but I guess it doesn't really matter which one to choose, but The user needs to know which (recipient, amount) pair failed and decide what to do about those failed pairs, If you look at the event right now, it just emits the event with the values that the function initially received as parameters. So I think it would be difficult for the user to find the failed pairs. If this problem is solved, I don't think it would matter either way. For example, revert(recipient.address) when it fails, so the user can see which one will be failed off-chain before sending tx.

[bayram98]

I prefer option 2, but I guess it doesn't really matter which one to choose, but The user needs to know which (recipient, amount) pair failed and decide what to do about those failed pairs, If you look at the event right now, it just emits the event with the values that the function initially received as parameters. So I think it would be difficult for the user to find the failed pairs. If this problem is solved, I don't think it would matter either way. For example, revert(recipient.address) when it fails, so the user can see which one will be failed off-chain before sending tx.

You mentioned really good point to remove the remain calculation in sendETH to save gas.

I will apply change, also put the revert(recipient.address)

[Shivansh070707]

pls check

[usgeeus]

@Shivansh070707 business logic question : sendERC20 function returns _amounts[i] to msg.sender if recipients[i] is address(0), sendETH function reverts if recipients[i] is address(0), Is this intentional? Or is this how you modeled it originally?

• if sendERC20 function returns _amounts[i] to msg.sender when recipients[i] is address(0), the event data will not match the actual transfer history.

[shivtokamak]

what should be the possible solution for this?

[usgeeus]

You can make sendERC20 function reverts if recipients[i] is address(0) Or You can make sendERc20 function emits the correct event data even when returning _amounts[i] to msg.sender

[Shivansh070707]

Please check Now

[usgeeus]

checked thanks

Another part: https://github.com/tokamak-network/Tms-contract/blob/9ad79997a1bdf649e1dbf3f590052922b5b94cdb/contracts/MultiSender.sol#L113-L115 I think this is unnecessary because safeTransferFrom will throw an error anyway.

[shivtokamak]

I removed that , please check

[usgeeus]

Thanks