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

Use ADR-28 addresses for module accounts #10225

Open
4 tasks
damiannolan opened this issue Sep 24, 2021 · 11 comments
Open
4 tasks

Use ADR-28 addresses for module accounts #10225

damiannolan opened this issue Sep 24, 2021 · 11 comments

Comments

@damiannolan
Copy link
Member

damiannolan commented Sep 24, 2021

Summary

Module Accounts created in /x/auth are currently using the following API from "github.com/tendermint/tendermint/crypto"

// NewModuleAddress creates an AccAddress from the hash of the module's name
func NewModuleAddress(name string) sdk.AccAddress {
	return sdk.AccAddress(crypto.AddressHash([]byte(name)))
}

Should module accounts now be created using the following outlined in ADR-28?

address.Module(moduleKey, []byte("some key"))

cc @colin-axner @robert-zaremba

Proposal

Replace crypto.AddressHash([]byte(name)) with address.Module(name, []byte(name))

Note: Unsure if this will cause any unwanted side-effects in other parts of the sdk(?)


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba
Copy link
Collaborator

New modules should use address.Module. This will be even more relevant if we will introduce ADR-33, where all inter-module communication will be "checked".

Yes, we should update (or even remove) NewModuleAddress and use address.Module instead (it's already implemented). @aaronc , do you think we can do it in 0.45?
It will require a migration.

@robert-zaremba robert-zaremba self-assigned this Oct 4, 2021
@alexanderbez
Copy link
Contributor

ACK, but we don't actually use address.Module anywhere in the SDK from what I can tell? Maybe we should update app.go?

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Oct 4, 2021

Yes, we didn't do any migration. To go forward, with the main thing from this issue, should we:

  1. remove NewModuleAddress
  2. deprecate NewModuleAddress (and update SDK to not use it in any place) and remove in the subsequent release (1.0).

I'm for option (2.)
In both cases we will do migration of Cosmos SDK core state.

@robert-zaremba
Copy link
Collaborator

I've started implementing option 2, but it seams we are using NewModuleAddress heavily in maccPerms and for BondedPool

@aaronc
Copy link
Member

aaronc commented Feb 8, 2022

I want us to distinguish two types of module accounts:

  • used internally and shouldn't receive coins. These probably shouldn't have an address at all and should be stored separately from balances. I suggest we call them module escrows. The majority of SDK use cases now are this
  • a module managing accounts like x/group or CosmWasm. These need an address and can receive coins and should use ADR 028

@robert-zaremba
Copy link
Collaborator

I suggest we call them module escrows

This suggests that it will receive coins. Users send money to escrow.

The use case is to use this address internally to hold coins. How about internal_address?

@aaronc
Copy link
Member

aaronc commented Feb 11, 2022

I suggest we call them module escrows

This suggests that it will receive coins. Users send money to escrow.

The use case is to use this address internally to hold coins. How about internal_address?

Sure, but it would be a separate type of balance in x/bank. Obviously that makes it a bigger change but I think it will avoid our current problems

@robert-zaremba
Copy link
Collaborator

OK, so user will never be exposed to those addresses. Good idea!

@julienrbrt
Copy link
Member

Looks this something useful to do for the x/auth abstraction (cc @facundomedica, @sontrinh16)

@sontrinh16
Copy link
Member

Looks this something useful to do for the x/auth abstraction (cc @facundomedica, @sontrinh16)

this would suggest we modify bank to store module account balance seperately, so we dont need an address for them maybe just account name

@aaronc
Copy link
Member

aaronc commented Oct 7, 2024

The alternative is on receive hooks in accounts to reject sends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants