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

Consider using account numbers from x/auth in state #15337

Open
aaronc opened this issue Mar 9, 2023 · 6 comments
Open

Consider using account numbers from x/auth in state #15337

aaronc opened this issue Mar 9, 2023 · 6 comments

Comments

@aaronc
Copy link
Member

aaronc commented Mar 9, 2023

Summary

New modules might consider using uint64 account numbers in storage rather than 20-32 bytes addresses if the motivation is reducing storage.

Problem Definition

This issue is in reference to the bech32 global removal work (#13140) in which we would pass an address.Codec to modules for converting addresses to/from strings instead of relying on AccAddress.String and AccAddressFromBech32 which rely on the global bech32 config.

The main motivation for modules to do the conversion from bech32 addresses to bytes is to use bytes in storage which saves space. However, if we really want to save space we could use uint64 account numbers which x/auth automatically creates. Bytes addresses are generally 20 to 32 bytes long - not very succinct if we want to reduce storage. uint64 addresses would be much more compact. If we're passing around address.Codec anyone to do the conversion, modules could go one step further and use the auth keeper to get the account number.

Proposal

I do not recommend that existing modules refactor state to reduce storage space using this approach. New modules may want to consider using account numbers in state rather than addresses.

We may also want to add methods to the auth keeper &/or query server to make this easier for modules, ex. GetAccountNumberForAddress(ctx context.Context, addr string) (uint64, error) and a reverse lookup method for getting the account by number.

@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Mar 9, 2023
@alexanderbez
Copy link
Contributor

Definitely worth considering, I'm not opposed to it! Although i see the two points as orthogonal. address.Codec will still be necessary, but in the context of x/auth, an integer can definitely make sense as the primary index 👍

@yihuang
Copy link
Collaborator

yihuang commented Mar 9, 2023

What's the implication for RemoveAccount and account with same address get created again, in that case, the new account has the same address but different account number.

@aaronc
Copy link
Member Author

aaronc commented Mar 9, 2023

As far as I know there is no logic currently to prune accounts. But if there were, yes that could be an issue

@tac0turtle tac0turtle added S:proposed and removed needs-triage Issue that needs to be triaged labels Mar 10, 2023
@robert-zaremba
Copy link
Collaborator

This could be in particular interesting for the storage keys, when we put address as a part of a key.

@robert-zaremba
Copy link
Collaborator

One downside is that we would need to always read the account to map address <---> number

BTW, why not combining account.Codec with codec.Codec? We could have a rule there for AccAddress type (as well for other types, VaAddress.

@robert-zaremba
Copy link
Collaborator

What's the implication for RemoveAccount and account with same address get created again, in that case, the new account has the same address but different account number.

In that case, I guess you should just reset the previous account, because the association address <---> number should stay.

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

No branches or pull requests

5 participants