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

custom-diff: x/bank #5

Closed
wants to merge 1 commit into from
Closed

Conversation

dongsam
Copy link

@dongsam dongsam commented Mar 11, 2022

BlockedAddr can only be configured at the time of init from app.go, so added it through kvstore and added the interface so that BlockedAddr can be added as needed even in the onchain environment.

@@ -1695,6 +1695,7 @@ GenesisState defines the bank module's genesis state.
| `balances` | [Balance](#cosmos.bank.v1beta1.Balance) | repeated | balances is an array containing the balances of all the accounts. |
| `supply` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) | repeated | supply represents the total supply. If it is left empty, then supply will be calculated based on the provided balances. Otherwise, it will be used to validate that the sum of the balances equals this amount. |
| `denom_metadata` | [Metadata](#cosmos.bank.v1beta1.Metadata) | repeated | denom_metadata defines the metadata of the differents coins. |
| `blocked_addrs` | [string](#string) | repeated | blocked_addrs is an array containing the blocked addresses |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `blocked_addrs` | [string](#string) | repeated | blocked_addrs is an array containing the blocked addresses |
| `blocked_addrs` | [string](#string) | repeated | blocked_addrs is an array containing the blocked addresses. |

@@ -22,6 +22,9 @@ message GenesisState {

// denom_metadata defines the metadata of the differents coins.
repeated Metadata denom_metadata = 4 [(gogoproto.moretags) = "yaml:\"denom_metadata\"", (gogoproto.nullable) = false];

// blocked_addrs is an array containing the blocked addresses
repeated string blocked_addrs = 5 [(gogoproto.moretags) = "yaml:\"blocked_addrs\"", (gogoproto.nullable) = false];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest another name: blocked_addresses, since using full Address instead of abbreviated Addr seems the sdk's convention.

}

// GetAllBlockedAddrs returns a list of blocked addresses.
func (k BaseSendKeeper) GetAllBlockedAddrs(ctx sdk.Context) (blockedAddrs []string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning []sdk.AccAddress is more intuitive.

// RemoveBlockedAddr delete a given address to blocked address map.
func (k BaseSendKeeper) RemoveBlockedAddr(ctx sdk.Context, addr sdk.AccAddress) {
store := ctx.KVStore(k.storeKey)
if store.Has(types.BlockedAddrKey(addr)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check if the key presents, since store.Delete will do nothing if the key doesn't exist.

return nil
}

// NewGenesisState creates a new genesis state.
func NewGenesisState(params Params, balances []Balance, supply sdk.Coins, denomMetaData []Metadata) *GenesisState {
func NewGenesisState(params Params, balances []Balance, supply sdk.Coins, denomMetaData []Metadata, blockedAddrs []string) *GenesisState {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about accepting blockedAddrs []sdk.AccAddress?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BankKeeper and others are receiving BlockedAddr as string, so string was used to follow this format as much as possible.

Ref. https://github.com/cosmos/cosmos-sdk/blob/4081bc3d0080644541ba3de34fbe1bde27779565/x/bank/keeper/keeper.go%23L96-L110

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original version used string, not sdk.AccAddress because it is used as key of a map.
Since sdk.AccAddress is []byte, the original author might thought that it's not suitable for map keys.
After changing the parameter type from map to slice, we can safely use []sdk.AccAddress again.
But as you said, to keep consistency between the original one, it's still possible to use []string as well.

@github-actions
Copy link

github-actions bot commented May 2, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label May 2, 2022
@github-actions github-actions bot closed this May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants