feat: v0.45.16 send restriction #173
Conversation
x/bank/keeper/send.go
Outdated
| @@ -105,6 +115,15 @@ func (k BaseSendKeeper) InputOutputCoins(ctx sdk.Context, inputs []types.Input, | |||
| return err | |||
| } | |||
|
|
|||
| for _, in := range inputs { | |||
There was a problem hiding this comment.
This could be inefficient with a nested for-loop. If the checks for the from and to addresses in sendCoinsRestrictionFn are independent, wouldn't it be possible to simplify it into a one-dimensional check by validating only the from address for inputs and only the to address for outputs?
(need to check if it is possible to handle an empty "from" or "to" address without errors, and add unit tests accordingly)
There was a problem hiding this comment.
Due to the bypass functionality for module accounts, it is deemed difficult to independently assess input addresses and output addresses.
However, an incorrect logic has been discovered in the current code where the restriction application is determined after subUnlockedCoins, so I will modify that part.
|
poorphd
force-pushed
the
v0.45.16/SendRestiction
branch
from
0287523
to
b2ae69d
Compare
The existing codebase had broken CI. This should be fixed in a separate PR. |
| } | ||
|
|
||
| // Check restrictions between the input address and the output address | ||
| outAddress, err = k.sendCoinsRestrictionFn(ctx, inAddress, outAddress, out.Coins) |
There was a problem hiding this comment.
The sum of the coins in the inputs and the sum of the coins in the outputs are equal, but each Coins in the input and output is independent. Therefore, using output.Coins as it is now leads to validation against an unintended amount.
Currently, we do not plan to deeply use the amount in the restriction function, so it's not a major issue. However, in the future, if we do use it, it may behave differently from what was intended. It might be a good idea to leave a TODO or comment in the code as a reminder.
There was a problem hiding this comment.
[Suggestion] How about seperate SendFromRestrictionFn and SendToRestrictionFn ?
Considering InputOutputCoins, it looks better logic to apply "from restriction" and "to restriction".
In minting case, it is better to use existing MintingRestrictionFn.
In burning case, it is allowed regardless of from or to.
If it is required to block a specific sender from sending to a specific receiver, I thnik that restriction function should not be applied to the InputOutputCoins function.
|
I remember hearing that in the previous localnet end-to-end test, even when restrictions occurred, errors were not exposed. Do you know if that issue has been resolved? Regardless of whether it's been fixed, it would be ideal if the test code could verify the occurrence of errors. |
The issue is resolved. If the transaction failed due to the restriction, the raw_log contains the error message. |
| @@ -1231,6 +1231,475 @@ func (suite *IntegrationTestSuite) TestMintCoinRestrictions() { | |||
| } | |||
| } | |||
|
|
|||
| func (suite *IntegrationTestSuite) TestSendRestrictions() { | |||
| type BankSendRestrictionFn func(ctx sdk.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.AccAddress, error) | |||
There was a problem hiding this comment.
[Suggestion] How about using existing type SendRestrictionFn in keeper.go
Is there any reason to define same type in test code?
Description
This PR contains send restriction features which can be enabled when initialize
BankKeeper.The basic functionality is implemented exactly as in Cosmos SDK v0.50, but while the existing implementation only applied restrictions to
SendCoins, this PR has been modified to apply restrictions toDelegateCoinsandUndelegateCoinsas well.Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...