x/bank SendCoins and SendRestriction

[wr1159]

Hi, I would like to know what happens in SendCoins if there is a SendRestriction that throws an exception - will the fromAddress lose the money permanently or will the SendCoins function terminate halfway through and rollback the subUnlockedCoins?

I have written a SendRestrictionFn in my module and during txs, the balances don't change. However, in my test files it seems like the coins are lost permanently and the total supply decreases which breaks the bank invariant but that might be related to my test harness.

In my SendRestrictionFn, certain address will throw an error. When I send coins from the blocked address (daemon tx bank send-coins blockedAddress normalAddress 1000atom), the tx instantly throws an error and doesn't get sent to the mempool. However, when I send coins to the blocked address (daemon tx bank send-coins normalAddress blockedAddress 1000atom), the tx goes through and only throws an error when querying the tx hash. Why is this? Is it a checkTx in the transaction lifecycle.

https://github.com/cosmos/cosmos-sdk/blob/c4d9a495052b78a02723ed1df1d336efe83a4e8d/x/bank/keeper/send.go#L208

[julienrbrt]

The coins are first deducted, and then we check the send restriction.
In the tx flow, is there is an error, then the tx is reverted.
In a module flow (that isn't in a tx, so begin/endblock call or whatever), the balances will indeed be deducted and then throw an error. This is why you need to run this in a cache context and only write it if it succeeds without an error.
If you want my opinion, I agree that we should swap those two conditions to avoid footguns.

[wr1159]

Thanks.

What was the original intention for applying sendRestriction after deduction of the coins from fromAddress?

Edit: When you say swap the 2 conditions, you mean check for SendRestriction then Deduct the coins from fromAddress right?

[julienrbrt]

Yes, no specific reasons. As I've said, if you use a cache context then it would be fine.

However I'd consider swapping the condition a useful improvement and would gladly see that in a PR for v0.51, as this is now the second time I hear this complain.

Are you willing to do that?

[wr1159]

Sure, I am willing to help! Can I check that the scope is going to be swapping those 2 lines and changing the documentation?

[julienrbrt]

Yes correct 👌

[wr1159]

I have opened a PR here cosmos/cosmos-sdk#20517