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

Prefix Account store key by type #6407

Closed
4 tasks
fedekunze opened this issue Jun 11, 2020 · 10 comments
Closed
4 tasks

Prefix Account store key by type #6407

fedekunze opened this issue Jun 11, 2020 · 10 comments
Labels
C:x/auth T: Dev UX UX for SDK developers (i.e. how to call our code) T: State Machine Breaking State machine breaking changes (impacts consensus).

Comments

@fedekunze
Copy link
Collaborator

fedekunze commented Jun 11, 2020

Summary

Add the account type to the Account key bytes to ease iteration over custom account types.

Problem Definition

Some apps implement a custom auth.Account (eg: ethermint's EthAccount or Kava's ValidatorVestingAccount). These apps usually have iteration logic over these accounts, but since Accounts are stored by their address on auth's AccountKeeper, they have to iterate over all of the accounts and cast the type to the concrete type in order to perform the logic.

Proposal

  • Add a Type() string function to the AccountI interface
  • Change the account key to the following:
// AddressStoreKey turn an address to key used to get it from the account store
func AddressStoreKey(accountType string, addr sdk.AccAddress) []byte {
	return append(AddressStoreKeyPrefix,  append(byte(accountType), addr.Bytes()...)...)
}
  • Add Iterator for a given account type

cc: @karzak @alexanderbez


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@fedekunze fedekunze added T:feature-request C:x/auth T: Dev UX UX for SDK developers (i.e. how to call our code) labels Jun 11, 2020
@fedekunze
Copy link
Collaborator Author

fedekunze commented Jun 11, 2020

I can work on this if there's consensus about the feature

@alexanderbez alexanderbez added the T: State Machine Breaking State machine breaking changes (impacts consensus). label Jun 11, 2020
@alexanderbez
Copy link
Contributor

I think this is reasonable, but note, you may be able to just use the bech32 HRP instead of introducing Type().

@karzak
Copy link
Contributor

karzak commented Jun 16, 2020

+1 to this idea - we created a keeper in validator vesting that just stores the addresses of these accounts to avoid iterating over everything - would be nice to be able to do it directly.

@alexanderbez
Copy link
Contributor

@fedekunze go for it, although I would check if there are more pressing items in the 0.39 board of higher priority before taking this on.

@fedekunze fedekunze self-assigned this Jun 16, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2020

This issue 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 Jul 4, 2020
@alexanderbez alexanderbez added pinned and removed stale labels Jul 4, 2020
@fedekunze fedekunze removed their assignment Jan 25, 2021
@fedekunze fedekunze added this to the v0.41 milestone Jan 25, 2021
@amaury1093
Copy link
Contributor

@fedekunze This is marked for v0.42, do you still want this for the next release? Do you have bandwidth to work on it?

@fedekunze
Copy link
Collaborator Author

Ideally we should include it for the next release. If someone could pick this that would be awesome 🙂

@aaronc
Copy link
Member

aaronc commented Feb 1, 2021

Hmm... I don't really want to add this sort of complexity to address keys. What you're asking for is basically a secondary index. So why not create a secondary index instead of trying to overload address keys with a secondary index function?

Also, I would just note that with ADR 028 there might be different ways we think about module accounts...

@fedekunze
Copy link
Collaborator Author

So why not create a secondary index instead of trying to overload address keys with a secondary index function?

Is there an example on how to implement a secondary index for a given key on the SDK?

@tac0turtle
Copy link
Member

closing this as it can already be done via indexes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/auth T: Dev UX UX for SDK developers (i.e. how to call our code) T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants