feat(x/bank): app wiring migration #12032

kocubinski · 2022-05-24T19:08:20Z

Description

Migrates x/bank module into the app wiring framework.

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...

Reviewers 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.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

kocubinski · 2022-05-31T17:04:42Z

simapp/test_helpers.go

+// ModuleAccountAddrs provides a list of blocked module accounts from configuration in app.yaml
+//
+// Ported from SimApp
+func ModuleAccountAddrs() map[string]bool {


Replaces SimApp.ModuleAccountAddrs for test function usages.

kocubinski · 2022-05-31T17:15:46Z

simapp/test_helpers.go

+func ModuleAccountAddrs() map[string]bool {
+	cfg := LoadAppConfig()
+	var blockedAddresses bank.BlockedAddresses
+	err := depinject.Inject(cfg, &blockedAddresses)


@aaronc Any thoughts on this integration of depinject into tests?

see my comments below. basically I think we want to get these from the bank keeper

How will an app dev provide/override these?

How will an app dev provide/override these?

cosmos-sdk/simapp/app.yaml

Lines 53 to 61 in 2f198b1

# uncomment to provide a block list different from the modules mentioned in auth.module_account_permissions

# blocked_module_accounts:

# - fee_collector

# - distribution

# - mint

# - bonded_tokens_pool

# - not_bonded_tokens_pool

# - gov

# - nft

x/bank/module.go

simapp/test_helpers.go

aaronc · 2022-05-31T19:04:27Z

x/bank/module.go

+
+type BlockedAddresses map[string]bool
+
+func provideBlockedAddresses(config *authmodulev1.Module) BlockedAddresses {


getting this from the auth module config is maybe reasonable. originally I had the blocked addresses specified in the bank module config but this is more safe. it would feel a bit more correct to have a method on the AccountKeeper to get these so we don't depend on a specific version of the auth module

I'm also not comfortable with providing these directly into the container like this.

A better way I think would be:

add AccountKeeper.GetModulePermissions()

add BankKeeper.GetBlockedAddresses()

@alexanderbez any opinion here?

AccountKeeper.GetModulePermissions() seems reasonable to me -- but they need to be configurable. Is that the case? Can a dev set these in the app configuration?

These permissions are set in the auth module config:

cosmos-sdk/simapp/app.yaml

Lines 33 to 44 in 33c4bac

module_account_permissions:

- account: fee_collector

- account: distribution

- account: mint

permissions: [ minter ]

- account: bonded_tokens_pool

permissions: [ burner, staking ]

- account: not_bonded_tokens_pool

permissions: [ burner, staking ]

- account: gov

permissions: [ burner ]

- account: nft

I think defaulting to AccountKeeper.GetModulePermissions() for blocked addresses makes sense, but let's also have a blocked addresses option in the bank config to override this if needed.

aaronc · 2022-05-31T19:05:44Z

simapp/test_helpers.go

+func ModuleAccountAddrs() map[string]bool {
+	cfg := LoadAppConfig()
+	var blockedAddresses bank.BlockedAddresses
+	err := depinject.Inject(cfg, &blockedAddresses)


see my comments below. basically I think we want to get these from the bank keeper

tac0turtle · 2022-06-01T13:45:58Z

ref #12083

tac0turtle · 2022-06-01T16:54:56Z

simapp/app.yaml

+    config:
+      "@type": cosmos.bank.module.v1.Module
+#      uncomment to provide a block list different from the modules mentioned in auth.module_account_permissions
+#      blocked_module_accounts:


above we have

module_account_permissions: - account: fee_collector - account: distribution

does this mean they can receive funds?

The default (existing) behavior is to disallow funds transfer to any of the module accounts in module_account_permissions including those you mentioned.

The block that is commented out here is new functionality, providing a way to override bank behavior to differ from auth configuration. Previously they were both being injected from code in SimApp.

We should remove the commented out block. We shouldn't encourage anyone to override this for now I think. Or is there any module account which can receive sends?

My thinking was that it might be good to demonstrate the full functionality of app.yaml in comments, kind of like the lein sample.project.clj but perhaps this is better as a documentation task, not the live SimApp config.

Yeah, we actually want to discourage this type of customization IMHO. This particular design issue has caused bugs. Honestly I can't wait to redesign it but for now we just need to limit damage

aaronc · 2022-06-01T18:06:10Z

proto/cosmos/bank/module/v1/module.proto

+    go_import: "github.com/cosmos/cosmos-sdk/x/bank"
+  };
+
+  // blocked_module_accounts configures exceptional module accounts which should be blocked from receiving funds


We need to document what the default is and that this overrides the default

aaronc · 2022-06-01T18:07:23Z

x/bank/module.go

+	depinject.In
+
+	Config        *modulev1.Module
+	AccountKeeper types.AccountKeeper `key:"cosmos.auth.module.v1.AccountKeeper"`


Suggested change

AccountKeeper types.AccountKeeper `key:"cosmos.auth.module.v1.AccountKeeper"`

AccountKeeper types.AccountKeeper `key:"cosmos.auth.v1.AccountKeeper"`

maybe we can skip module in the name?

aaronc · 2022-06-01T18:07:44Z

x/auth/keeper/keeper.go

@@ -148,6 +151,11 @@ func (ak AccountKeeper) GetNextAccountNumber(ctx sdk.Context) uint64 {
 	return accNumber
 }

+// GetModulePermissions fetches per-module account permissions


Suggested change

// GetModulePermissions fetches per-module account permissions

// GetModulePermissions fetches per-module account permissions.

aaronc · 2022-06-01T18:08:32Z

simapp/test_helpers.go

+	cfg := appconfig.LoadYAML(AppConfigYaml)
+	var bk bankkeeper.Keeper
+	err := depinject.Inject(cfg, &bk)


Suggested change

cfg := appconfig.LoadYAML(AppConfigYaml)

var bk bankkeeper.Keeper

err := depinject.Inject(cfg, &bk)

var bk bankkeeper.Keeper

err := depinject.Inject(AppConfig, &bk)

No need to re-read the yaml. I think this is the pattern we should encourage

aaronc · 2022-06-01T18:09:15Z

simapp/app.go


-var appConfig = appconfig.LoadYAML(appConfigYaml)
+var appConfig = appconfig.LoadYAML(AppConfigYaml)


Suggested change

var appConfig = appconfig.LoadYAML(AppConfigYaml)

var AppConfig = appconfig.LoadYAML(appConfigYaml)

aaronc · 2022-06-01T18:09:32Z

simapp/app.go

@@ -198,9 +198,9 @@ func init() {
 }

 //go:embed app.yaml
-var appConfigYaml []byte
+var AppConfigYaml []byte


Suggested change

var AppConfigYaml []byte

var appConfigYaml []byte

feels more logical to export AppConfig and not the yaml

… SimApp

* Integrate with keyed resolvers feature of depinject

aaronc · 2022-06-01T18:50:02Z

proto/cosmos/bank/module/v1/module.proto

+  };
+
+  // blocked_module_accounts configures exceptional module accounts which should be blocked from receiving funds
+  repeated string blocked_module_accounts = 1;


Suggested change

repeated string blocked_module_accounts = 1;

repeated string blocked_module_accounts_override = 1;

aaronc · 2022-06-01T19:48:13Z

proto/cosmos/bank/module/v1/module.proto

+  };
+
+  // blocked_module_accounts configures exceptional module accounts which should be blocked from receiving funds.
+  // If left empty defaults to the list of account names supplied in the auth module configuration as


Suggested change

// If left empty defaults to the list of account names supplied in the auth module configuration as

// If left empty it defaults to the list of account names supplied in the auth module configuration as

codecov · 2022-06-01T19:54:56Z

Codecov Report

Merging #12032 (526f390) into main (f71d464) will increase coverage by 0.33%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##             main   #12032      +/-   ##
==========================================
+ Coverage   66.03%   66.36%   +0.33%     
==========================================
  Files         671      685      +14     
  Lines       70956    71983    +1027     
==========================================
+ Hits        46859    47775     +916     
- Misses      21435    21517      +82     
- Partials     2662     2691      +29

Impacted Files	Coverage Δ
depinject/inject.go	`76.00% <ø> (ø)`
simapp/test_helpers.go	`26.34% <0.00%> (-0.46%)`	⬇️
x/bank/keeper/send.go	`82.14% <0.00%> (-0.85%)`	⬇️
x/bank/module.go	`63.91% <76.92%> (+5.58%)`	⬆️
simapp/app.go	`82.64% <100.00%> (-0.38%)`	⬇️
x/auth/keeper/keeper.go	`70.27% <100.00%> (+0.54%)`	⬆️
x/auth/module.go	`63.29% <100.00%> (ø)`
depinject/location.go	`73.58% <0.00%> (ø)`
depinject/container.go	`89.08% <0.00%> (ø)`
... and 11 more

* Initial work on bank module app wiring * Fix import in bank module * integrating bank module into simapp DI * Integrate usages of ModuleAccountAddrs into DI container, remove from SimApp * Remove dependency on authkeeper from bank module * Integrate with keyed resolvers feature of depinject * Refactoring to remove direct dependency from bank -> auth * Clean up app.yaml usage in test * Clean up comments, keys, and testing fns. * Remove commented example in bank module config * Regenerate code from proto files * Fix usage of BlockedModuleAccountsOverride

github-actions bot added C:x/auth C:x/bank C:x/params labels May 24, 2022

kocubinski changed the title ~~Kocubinski/app wiring bank~~ feat(x/bank): app wiring migration May 24, 2022

kocubinski mentioned this pull request May 24, 2022

EPIC: Separate all SDK modules into standalone go modules #11899

Closed

81 tasks

github-actions bot removed the C:x/params label May 26, 2022

kocubinski force-pushed the kocubinski/app-wiring-bank branch 2 times, most recently from 565ae53 to 2e06e78 Compare May 31, 2022 14:48

github-actions bot added C:x/auth and removed C:x/auth labels May 31, 2022

kocubinski commented May 31, 2022

View reviewed changes

kocubinski marked this pull request as ready for review May 31, 2022 17:05

kocubinski requested a review from a team as a code owner May 31, 2022 17:05

kocubinski force-pushed the kocubinski/app-wiring-bank branch from eb85234 to 57b0eae Compare May 31, 2022 17:06

kocubinski commented May 31, 2022

View reviewed changes

tac0turtle reviewed May 31, 2022

View reviewed changes

x/bank/module.go Outdated Show resolved Hide resolved

aaronc reviewed May 31, 2022

View reviewed changes

kocubinski force-pushed the kocubinski/app-wiring-bank branch 2 times, most recently from 8e4b75a to da280d6 Compare June 1, 2022 15:54

kocubinski requested review from aaronc and alexanderbez June 1, 2022 16:25

kocubinski force-pushed the kocubinski/app-wiring-bank branch from 5409d22 to 2f198b1 Compare June 1, 2022 16:26

tac0turtle reviewed Jun 1, 2022

View reviewed changes

aaronc reviewed Jun 1, 2022

View reviewed changes

kocubinski added 5 commits June 1, 2022 14:44

Initial work on bank module app wiring

8c8507e

Fix import in bank module

2fc1cca

integrating bank module into simapp DI

57cdf78

Integrate usages of ModuleAccountAddrs into DI container, remove from…

0a0b7cf

… SimApp

Remove dependency on authkeeper from bank module

181c0a5

* Integrate with keyed resolvers feature of depinject

kocubinski added 3 commits June 1, 2022 14:44

Refactoring to remove direct dependency from bank -> auth

1fe3585

Clean up app.yaml usage in test

46016e6

Clean up comments, keys, and testing fns.

02a266a

kocubinski force-pushed the kocubinski/app-wiring-bank branch from 6232d98 to 02a266a Compare June 1, 2022 19:44

kocubinski requested a review from aaronc June 1, 2022 19:45

aaronc reviewed Jun 1, 2022

View reviewed changes

Remove commented example in bank module config

93562d1

aaronc approved these changes Jun 1, 2022

View reviewed changes

Regenerate code from proto files

0862140

aaronc requested a review from facundomedica June 1, 2022 19:51

Fix usage of BlockedModuleAccountsOverride

526f390

facundomedica approved these changes Jun 1, 2022

View reviewed changes

kocubinski merged commit 8c281ee into main Jun 1, 2022

kocubinski deleted the kocubinski/app-wiring-bank branch June 1, 2022 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/bank): app wiring migration #12032

feat(x/bank): app wiring migration #12032

kocubinski commented May 24, 2022 •

edited

Loading

kocubinski May 31, 2022

kocubinski May 31, 2022

aaronc May 31, 2022

alexanderbez May 31, 2022

kocubinski Jun 1, 2022

aaronc May 31, 2022

alexanderbez May 31, 2022

aaronc May 31, 2022 •

edited

Loading

aaronc May 31, 2022

aaronc May 31, 2022

tac0turtle commented Jun 1, 2022

tac0turtle Jun 1, 2022

kocubinski Jun 1, 2022

kocubinski Jun 1, 2022 •

edited

Loading

aaronc Jun 1, 2022

kocubinski Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

aaronc Jun 1, 2022

codecov bot commented Jun 1, 2022 •

edited

Loading

	# uncomment to provide a block list different from the modules mentioned in auth.module_account_permissions
	# blocked_module_accounts:
	# - fee_collector
	# - distribution
	# - mint
	# - bonded_tokens_pool
	# - not_bonded_tokens_pool
	# - gov
	# - nft


		type BlockedAddresses map[string]bool

		func provideBlockedAddresses(config *authmodulev1.Module) BlockedAddresses {

	module_account_permissions:
	- account: fee_collector
	- account: distribution
	- account: mint
	permissions: [ minter ]
	- account: bonded_tokens_pool
	permissions: [ burner, staking ]
	- account: not_bonded_tokens_pool
	permissions: [ burner, staking ]
	- account: gov
	permissions: [ burner ]
	- account: nft

	AccountKeeper types.AccountKeeper `key:"cosmos.auth.module.v1.AccountKeeper"`
	AccountKeeper types.AccountKeeper `key:"cosmos.auth.v1.AccountKeeper"`

	// GetModulePermissions fetches per-module account permissions
	// GetModulePermissions fetches per-module account permissions.


		var appConfig = appconfig.LoadYAML(appConfigYaml)
		var appConfig = appconfig.LoadYAML(AppConfigYaml)

	repeated string blocked_module_accounts = 1;
	repeated string blocked_module_accounts_override = 1;

	// If left empty defaults to the list of account names supplied in the auth module configuration as
	// If left empty it defaults to the list of account names supplied in the auth module configuration as

feat(x/bank): app wiring migration #12032

feat(x/bank): app wiring migration #12032

Conversation

kocubinski commented May 24, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

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

aaronc May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocubinski Jun 1, 2022 • edited Loading

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

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

codecov bot commented Jun 1, 2022 • edited Loading

Codecov Report

kocubinski commented May 24, 2022 •

edited

Loading

aaronc May 31, 2022 •

edited

Loading

kocubinski Jun 1, 2022 •

edited

Loading

codecov bot commented Jun 1, 2022 •

edited

Loading