Enable/disable coin transfers by denom #6527

iramiller · 2020-06-27T00:32:42Z

Description

This PR extends the module properties of the Bank module to support setting a SendEnabled flag on specific coin denominations. The implementation was designed such that a default SendEnabled flag can be created for all denominations. This behavior was used to maintain the existing behavior of SendEnabled being set to True for all coin denominations.

Unit tests have been created around the extended parameter definitions to full cover all added behaviors for single sendenabled properties as well as the array of send enabled properties. The simulation test have been modified to match the updated API while maintaining the original behavior of setting the SendEnabled flag to false roughly 5% of the time.
closes: #6518

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

codecov · 2020-06-27T00:42:56Z

Codecov Report

Merging #6527 into master will increase coverage by 0.85%.
The diff coverage is 61.92%.

@@            Coverage Diff             @@
##           master    #6527      +/-   ##
==========================================
+ Coverage   55.60%   56.45%   +0.85%     
==========================================
  Files         457      412      -45     
  Lines       27440    24503    -2937     
==========================================
- Hits        15257    13834    -1423     
+ Misses      11083     9578    -1505     
+ Partials     1100     1091       -9

Modified SendEnabled property to be part of generic Params object Updated genesis functions to use default params structure

fedekunze

Looks good! I left a couple of comments and a few questions

x/bank/handler.go

x/bank/types/params.go

Remove empty denom capability from SendEnabled parameters Update simulation to exercise both configuration options independently

x/bank/types/params.go

fedekunze

ACK. Thanks, @iramiller. Minor suggestions and then I'll approve 👍

x/bank/types/params_test.go

proto/cosmos/bank/bank.proto

x/bank/types/params.go

proto/cosmos/bank/bank.proto

alexanderbez

Excellent work. I think this changeset makes sense. I left a few comments around nomenclature and UX, but overall looks great.

proto/cosmos/bank/bank.proto

x/bank/keeper/send.go

alexanderbez · 2020-07-07T00:24:23Z

x/bank/keeper/send.go

+	SetParams(ctx sdk.Context, params types.Params)
+
+	GetSendEnabled(ctx sdk.Context, denom string) bool
+	CoinsSendEnabled(ctx sdk.Context, coins ...sdk.Coin) error


Suggested change

CoinsSendEnabled(ctx sdk.Context, coins ...sdk.Coin) error

SendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error

On second thought, I'm thinking this should rather take a variadic list of denominations (...string) and be called SendEnabledDenoms to complement SendEnabledDenom.

After reviewing the current usages of these methods, in all cases the arguments for denom are passed in using coin.Denom. In addition using the denom string means that when a structure of coins is present the denom string must be extracted and passed into these functions (often adding yet another for-range loop). Based on this review and the existing methods which are all based on sdk.Coin it would seem that for consistency we should be using the Coin type and not a denom string. Further a denom string may not be a valid denom at all when it is handled outside of the Coin class.

You can assume all denoms are valid, and you can easily implement a method Coins#Denominations that returns a unique list, so then it becomes SendEnabledDenoms(coins.Denoms()).

alexanderbez · 2020-07-07T00:25:09Z

x/bank/keeper/send.go

+	GetParams(ctx sdk.Context) types.Params
+	SetParams(ctx sdk.Context, params types.Params)
+
+	GetSendEnabled(ctx sdk.Context, denom string) bool


Suggested change

GetSendEnabled(ctx sdk.Context, denom string) bool

SendEnabledDenom(ctx sdk.Context, denom string) bool

Related to above, method name order was changed however current implementation unifies on Coin as the parameter type in order to match all the other bank send keeper methods and improve developer ergonomics.

alexanderbez · 2020-07-07T00:33:50Z

x/bank/spec/05_params.md

+denominations to their send_enabled status.  Entries in this list take
+precedence over the `DefaultSendEnabled` setting.
+
+## DefaultSendEnabled


I'm not really convinced this is necessary or provides a cleaner UX. What is the drawback to just using SendEnabled? I can see people getting confused over this IMHO.

x/bank/types/params.go

AdityaSripal

Great work!! Changes make sense to me.

Though one thing to note, if the DefaultSendEnabled = false, then all ibc transfers will not be sendable by default since they will have new denoms. Is this desirable behaviour?

iramiller · 2020-07-07T05:03:39Z

I will provide some additional follow up tomorrow (along with the requested usability/naming changes) but I wanted to provide my reasoning for the separate default setting from the coin specific ones now while the team members in the EU will see it.

The inclusion of a default send enabled parameter preserves the existing behaviors for any deployments that may depend on it. For my team’s specific use cases we will largely use the default enabled true setting and only set a small subset of coins to false (out of many).

In addition the separation of the default send enabled flag from the coin specific versions allow the system to be configured in either the ‘allow list’ or ‘deny list’ style depending on the security style favored. This type of flexibility seems useful for blocking all unexpected coins for example (IBC?).

colin-axner · 2020-07-07T09:26:22Z

In addition the separation of the default send enabled flag from the coin specific versions allow the system to be configured in either the ‘allow list’ or ‘deny list’ style depending on the security style favored. This type of flexibility seems useful for blocking all unexpected coins for example (IBC?).

This justification in regards to IBC makes sense to me. I'm fairly certain without DefaultSendEnabled all coins being minted from IBC would be blocked except with a gov proposal. ICS20 (and similar IBC applications) could also be updated to set params on creation of a new coin, but I think that would be undesirable. cc/ @cwgoes

--

Code looks good 👍 thanks for making the SDK better!

fedekunze · 2020-07-07T11:36:54Z

Note regarding IBC: the ibc-transfer module needs to additionally create a parameter for enabling/disabling cross-chain transfers. This is bc some chains might want to enable local chain transfers but disable them to other chains. See #6591 for more context

ICS20 (and similar IBC applications) could also be updated to set params on creation of a new coin, but I think that would be undesirable.

@AdityaSripal you can enable the TransfersEnabled boolean parameter in ibc-transfer and disable single coin cross-chain transfers (with the full denom prefix) with the bank's SendEnabled parameter.

Alternatively, we could add a regex validation on #6591 params to enable token denoms that match the given regex

…com:iramiller/cosmos-sdk into 6518-enable-disable-coin-transfers-by-denom

alexanderbez

Something with the nomenclature and usage of params just doesn't sit right with me for some reason (e.g. default shouldn't really be used in a param name). But I can't come up with anything cleaner or better, so I'll give an ACK here.

addressed

fedekunze · 2020-07-08T17:10:54Z

CHANGELOG.md

@@ -127,6 +127,7 @@ be used to retrieve the actual proposal `Content`. Also the `NewMsgSubmitProposa
 * (modules) [\#6311](https://github.com/cosmos/cosmos-sdk/issues/6311) Remove `alias.go` usage
 * (x/auth) [\#6443](https://github.com/cosmos/cosmos-sdk/issues/6443) Move `FeeTx` and `TxWithMemo` interfaces from `x/auth/ante` to `types`.
 * (modules) [\#6447](https://github.com/cosmos/cosmos-sdk/issues/6447) Rename `blacklistedAddrs` to `blockedAddrs`.
+* (x/bank) [\6518](https://github.com/cosmos/cosmos-sdk/pull/6518) Support for per-denomination send_enabled flags.  Existing send_enabled flag has been moved into a Params structure and transformed into an array of: `{denom: string, enabled: bool}`


can you move this to State Machine Breaking? 🙏

aaronc · 2020-07-10T17:25:03Z

x/bank/keeper/send.go

-	GetSendEnabled(ctx sdk.Context) bool
-	SetSendEnabled(ctx sdk.Context, enabled bool)
+	GetParams(ctx sdk.Context) types.Params
+	SetParams(ctx sdk.Context, params types.Params)


Why did we add SetParams to SendKeeper? This breaks ocaps even more. Just noting that this should be placed elsewhere at some point in the future.

* initial implementation of per denom sendenabled * Fix for accidentally removed keyword * Validate individual param in param array * Lint fix * Refactor bank params to use protobuf Modified SendEnabled property to be part of generic Params object Updated genesis functions to use default params structure * Refactor simulation genesis for clarity * update changelog for bank sendenable per denom * fix NoOpMsg type in multisend test * Add a coin denom send check utility function * Additional godoc comments and clarification * Add default send enabled parameter to bank. Remove empty denom capability from SendEnabled parameters Update simulation to exercise both configuration options independently * Minor suggested improvements. * simulation fix * bank proto sendenabled package name removed * Remove extra gogo proto yaml tags * Params rename IsSendEnabled to SendEnabledDenom * Refactor to SendEnabledCoin(s) * update slashing test to use bank params * Clean up change log entry for feature. Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Ira Miller added 2 commits June 26, 2020 17:09

initial implementation of per denom sendenabled

3c3a232

Fix for accidentally removed keyword

5cb0357

Ira Miller and others added 7 commits June 26, 2020 19:04

Validate individual param in param array

b1ea0e2

Lint fix

03ae118

Refactor bank params to use protobuf

fba1a1a

Modified SendEnabled property to be part of generic Params object Updated genesis functions to use default params structure

Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom

fe4844b

Refactor simulation genesis for clarity

dbf96c3

update changelog for bank sendenable per denom

d05321f

fix NoOpMsg type in multisend test

8568819

iramiller marked this pull request as ready for review June 30, 2020 02:25

iramiller requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners June 30, 2020 02:25

iramiller changed the title ~~initial implementation of per denom sendenabled~~ Extend bank module sendEnabled flag with per denomination support Jun 30, 2020

iramiller changed the title ~~Extend bank module sendEnabled flag with per denomination support~~ Extend bank module sendEnabled flag with per denom support Jun 30, 2020

iramiller changed the title ~~Extend bank module sendEnabled flag with per denom support~~ Enable/disable coin transfers by denom Jun 30, 2020

fedekunze added genesis-breaking T: State Machine Breaking State machine breaking changes (impacts consensus). C:x/bank labels Jun 30, 2020

fedekunze reviewed Jun 30, 2020

View reviewed changes

Ira Miller added 3 commits June 30, 2020 09:05

Add a coin denom send check utility function

9308435

Additional godoc comments and clarification

a76d751

Add default send enabled parameter to bank.

42d8c08

Remove empty denom capability from SendEnabled parameters Update simulation to exercise both configuration options independently

iramiller commented Jun 30, 2020

View reviewed changes

x/bank/types/params.go Show resolved Hide resolved

fedekunze reviewed Jun 30, 2020

View reviewed changes

x/bank/types/params_test.go Outdated Show resolved Hide resolved

proto/cosmos/bank/bank.proto Outdated Show resolved Hide resolved

x/bank/types/params.go Outdated Show resolved Hide resolved

x/bank/types/params.go Outdated Show resolved Hide resolved

fedekunze reviewed Jul 6, 2020

View reviewed changes

proto/cosmos/bank/bank.proto Outdated Show resolved Hide resolved

Ira Miller and others added 3 commits July 6, 2020 09:29

simulation fix

1f753e6

bank proto sendenabled package name removed

f06e854

Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom

e11d1d1

fedekunze requested review from AdityaSripal, colin-axner and alessio July 6, 2020 16:24

alexanderbez removed the R4R label Jul 7, 2020

alexanderbez suggested changes Jul 7, 2020

View reviewed changes

AdityaSripal approved these changes Jul 7, 2020

View reviewed changes

fedekunze and others added 7 commits July 7, 2020 13:45

Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom

7d63967

Remove extra gogo proto yaml tags

23494e8

Params rename IsSendEnabled to SendEnabledDenom

d25c8f9

Refactor to SendEnabledCoin(s)

9c8918d

Merge branch '6518-enable-disable-coin-transfers-by-denom' of github.…

c7f1902

…com:iramiller/cosmos-sdk into 6518-enable-disable-coin-transfers-by-denom

update slashing test to use bank params

78b521b

Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom

9458c82

alexanderbez approved these changes Jul 8, 2020

View reviewed changes

Merge branch 'master' into 6518-enable-disable-coin-transfers-by-denom

8778d26

fedekunze reviewed Jul 8, 2020

View reviewed changes

Clean up change log entry for feature.

eda215b

fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 8, 2020

mergify bot merged commit 589c1a5 into cosmos:master Jul 8, 2020

iramiller deleted the 6518-enable-disable-coin-transfers-by-denom branch July 8, 2020 18:26

aaronc reviewed Jul 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable/disable coin transfers by denom #6527

Enable/disable coin transfers by denom #6527

iramiller commented Jun 27, 2020 •

edited

Loading

codecov bot commented Jun 27, 2020 •

edited

Loading

fedekunze left a comment

fedekunze left a comment

alexanderbez left a comment

alexanderbez Jul 7, 2020

alexanderbez Jul 7, 2020

iramiller Jul 7, 2020

alexanderbez Jul 8, 2020

alexanderbez Jul 7, 2020

iramiller Jul 7, 2020

alexanderbez Jul 7, 2020

AdityaSripal left a comment

iramiller commented Jul 7, 2020

colin-axner commented Jul 7, 2020

fedekunze commented Jul 7, 2020 •

edited

Loading

alexanderbez left a comment

fedekunze Jul 8, 2020

aaronc Jul 10, 2020

	CoinsSendEnabled(ctx sdk.Context, coins ...sdk.Coin) error
	SendEnabledCoins(ctx sdk.Context, coins ...sdk.Coin) error

	GetSendEnabled(ctx sdk.Context, denom string) bool
	SendEnabledDenom(ctx sdk.Context, denom string) bool

Enable/disable coin transfers by denom #6527

Enable/disable coin transfers by denom #6527

Conversation

iramiller commented Jun 27, 2020 • edited Loading

Description

codecov bot commented Jun 27, 2020 • edited Loading

Codecov Report

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez 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

AdityaSripal left a comment

Choose a reason for hiding this comment

iramiller commented Jul 7, 2020

colin-axner commented Jul 7, 2020

fedekunze commented Jul 7, 2020 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iramiller commented Jun 27, 2020 •

edited

Loading

codecov bot commented Jun 27, 2020 •

edited

Loading

fedekunze commented Jul 7, 2020 •

edited

Loading