Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Bank MsgMultiSend #12397

Closed
4 tasks
sunnya97 opened this issue Jun 30, 2022 · 9 comments · Fixed by osmosis-labs/cosmos-sdk#274
Closed
4 tasks

Remove Bank MsgMultiSend #12397

sunnya97 opened this issue Jun 30, 2022 · 9 comments · Fixed by osmosis-labs/cosmos-sdk#274
Labels

Comments

@sunnya97
Copy link
Member

sunnya97 commented Jun 30, 2022

I'd like to propose that we remove the Bank module's MsgMultiSend msg type. I believe it was created before MultiMsg txs were created, and it serves mostly duplicate usage. I don't believe the message type has any significant usage in any application, and is mostly just a legacy feature we're maintaining.

Part of the motive for this is that I want to add a hook to the Send functionality of the bank module, but the MsgMultiSend bypasses it because it doesn't use the SendCoins function of the Bank keeper and instead uses a SubtractCoins and AddCoins.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

ACK

@tac0turtle
Copy link
Member

reopen until this lands in the sdk

@codehans
Copy link

codehans commented Jul 4, 2022

MsgMultiSend has a more compact representation than using multimsg, so it makes it more compatible with Ledger devices. You can get 4-5 msgs in a tx on Ledger, but IIRC 30ish recipients in a MsgMultiSend.

@alexanderbez
Copy link
Contributor

The problem it's maintenance overhead for a message that no one really uses.

@codehans
Copy link

codehans commented Jul 4, 2022

The problem it's maintenance overhead for a message that no one really uses.

We use it! We built a whole app around it on Terra. In the nicest way this is a seriously small amount of discussion to add a breaking change to an SDK that dozens of chains rely on

@alexanderbez
Copy link
Contributor

Ok, maybe we keep it then. Should we close the PR @julienrbrt ?

@julienrbrt
Copy link
Member

As per this discussion, closing.

@sunnya97
Copy link
Member Author

sunnya97 commented Jul 6, 2022

I agree, if there are active users of it, we shouldn't remove it completely

@codehans what's the use case that you guys are using it for? It mostly for multi-recipient or also multi-sender?

The biggest issue with it rn is that it is both multi-sender and multi-recipient, and what it does is it does a bankkeeper.SubtractCoins from all the senders and bankkeeper.AddCoins to all the recipients (after verifying that the sums are the same)

However, this means it doesn't use the "normal" bankkeeper.SendCoins functionality.

I'm trying to add a hook to the SendCoins function that can allow modules/CosmWasm contracts to add a hook to a coin transfer, but the MultiSend with its lack of 1:1 of recipient:sender makes that not possible, allowing people to bypass the hook

Would something modifying/creating a new msg to do a single sender but many recipients solve your use case, cause in that case we can add the hooks

@codehans
Copy link

codehans commented Jul 7, 2022

@sunnya97 yeha typically only a single input

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants