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

ModuleAccount addresses don't follow ADR-028 #13782

Closed
amaury1093 opened this issue Nov 7, 2022 · 1 comment · Fixed by #14017
Closed

ModuleAccount addresses don't follow ADR-028 #13782

amaury1093 opened this issue Nov 7, 2022 · 1 comment · Fixed by #14017
Labels
T:Bug T: State Machine Breaking State machine breaking changes (impacts consensus).

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Nov 7, 2022

Summary of Bug

The module account addresses we currently have don't follow ADR-028. I think we simply never bothered to migrate them to ADR-028. Currently, for example for "gov", the module address is:

"gov": cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn

However, ADR-028 defines the module account addresses as:

In Basic Address section we defined a module account address as:

address.Hash("module", moduleName)

which gives:

"gov": cosmos1yshfvvjkr5rp6sqykvsy75jqcy25sqj57xrg3ued5cdzdspx7ldq2w8xv0

Version

6ae5641

Steps to Reproduce

func TestModuleAccounts(t *testing.T) {
	// Current module addresses for gov.
	require.Equal(t,
		"cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
		sdk.AccAddress(crypto.AddressHash([]byte("gov"))).String(),
	)

	// Correct ADR-028 addresses for gov.
	require.Equal(t,
		"cosmos1yshfvvjkr5rp6sqykvsy75jqcy25sqj57xrg3ued5cdzdspx7ldq2w8xv0",
		sdk.AccAddress(address.Hash("module", []byte("gov"))).String(),
	)
}

Also, the ModuleAccount's Validate function is incorrect in validating its address:

if ma.Address != sdk.AccAddress(crypto.AddressHash([]byte(ma.Name))).String() {
return fmt.Errorf("address %s cannot be derived from the module name '%s'", ma.Address, ma.Name)
}

It should use the address.Hash("module", moduleName) function.

Proposal

  • Change the ModuleAccount validation function to use ADR-028 (i.e. with "module" prefix)
  • Do a migration to migrate all addresses

Alternative proposal

We could also change ADR-028 to not use the "module" prefix for module acct addresses. I'm currently not sure of the security implications of this though.

@julienrbrt
Copy link
Member

This has been fixed in #14017 where simplifying the ADR have been the solution opted for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Bug T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants