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

feat: add vesting account handling #232

Merged
merged 41 commits into from
Oct 20, 2021

Conversation

huichiaotsou
Copy link
Contributor

@huichiaotsou huichiaotsou commented Oct 15, 2021

Description

This PR replaces #158
close #138

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Re-reviewed Files changed in the Github PR explorer.

database/schema/01-auth.sql Outdated Show resolved Hide resolved
database/schema/01-auth.sql Outdated Show resolved Hide resolved
database/schema/01-auth.sql Outdated Show resolved Hide resolved
modules/auth/types.go Outdated Show resolved Hide resolved
types/auth.go Outdated
// --------------- For Vesting Account ---------------

// Account represents a chain account
type VestingAccount struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, why are these re-declared since they are already available inside the SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @RiccardoM , I added the types on the side because I couldn't really get the account type with Codec.. so I did with brute force to parse them into json. Do you know a way how I can parse/unpack the accounts?

Copy link
Contributor

@RiccardoM RiccardoM Oct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huichiaotsou In order to understand how to unpack accounts, it's worth to understand how they are handled from Cosmos and why they are packed in a strange way. The best way to do this is though a series of considerations:

  1. All the data returned from a gRPC call is serialized using Protobuf
  2. In order to represent a generic account, inside the Cosmos SDK there are multiple Go interfaces used. The most common are:
    1. AccountI, to represent a generic account
    2. VestingAccount to represent a generic vesting account
  3. Protobuf does not support Go interfaces by default. For this reason, the Cosmos team has decided to use the Any type to serialize them.

So, in order to read the details of a vesting account you need to do the following:

  1. Unpack that account from Any into an AccountI interface:
var accountI authtypes.AccountI
err := cdc.UnpackAny(account, &accountI)
if err != nil {
  return err
}
  1. Check if that accountI is also implementing the VestingAccountI interface:
vestingAccount, ok := accountI.(exported.VestingAccount)
if !ok {
  // The account is not a VestingAccount. Do as you wish (skip the for loop, return an error, ...)
}

From there, you can use vestingAccount however you wish since it will be of type VestingAccount.

Side note

Since they can be of various types (ContinousVestingAccount, DelayedVestingAccount and PeriodicVestingAccount), the best way to store VestingAccount instances is to simply serialize them as JSON and store them inside a table. However, this might make it really hard to get some useful insights that we might want to display on BigDipper (such as total vested/unvested tokens, next unvesting period, etc).

For this reason, I would like to ask @ryuash and @calvinkei how they are going to use the vesting data in order to decide how to store them accordingly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default Big Dipper 2.0 will not be using this data in favor of Forbole X.
Inside the Forbole X wallet details page I would assume it would show xx tokens are locked until xx (maybe a countdown type of animation)

Maybe @calvinkei can clarify

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryuash Wouldn't it be interesting for BigDipper to show a global amount of locked/unlocked tokens? Asking @kwunyeung also.

That might be interesting for users, no? To know when the next large unlock will happen or other interesting things

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the UI for showing vesting tokens on forbole x
Screenshot 2021-10-18 at 3 30 30 PM

So, it needs to support "total vesting tokens" and pagination. Probably filter & sort by date as well

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahs, if we do some feature like that then we definitely cannot throw a JSONB in to psql.

I would need to get all possible vesting periods in the genesis file and query by xx time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huichiaotsou In order to preserve forward-compatibility with new features that might come up I would suggest then adding a better handling for different vesting account types. We can do so by doing this:

// inside handler
db.StoreVestingAccount(vestingAccount)

// -------------------------

func (db *Database) StoreVestingAccount(vestingAccount exported.VestingAccount) error {
  if continousVestingAccount, ok := vestingAccount.(*vestingtypes.ContinousVestingAccount); ok {
    return db.storeContinousVestingAccount(continousVestingAccount)
  }

  if delayedVestingAccount, ok := vestingAccount.(*vestingtypes.DelayedVestingAccount); ok {
    return db.storeDelayedVestingAccount(continousVestingAccount)
  }

  if periodicVestingAccount, ok := vestingAccount.(*vestingtypes.PeriodicVestingAccount); ok {
    return db.storePeriodicVestingAccount(continousVestingAccount)
  }

  return fmt.Errorf("invalid vesting account type: %T", vestingAccount")

This way we can handling the storing of different types properly, with various periods etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RiccardoM I'm also interested in showing vesting amount on BD but I do agree with @ryuash that it could be in favour of Forbole X at some point if BD not showing them by default. I think we can have this feature showing on X first. I also wanna see if there will be any feedback or request on getting this feature on BD.

Agree not storing the vesting periods in JSONB as the structure is very clear that each record has an array of coins and a vested date. It's not any arbitrary json content where we may search in certain keys inside the json.

@huichiaotsou huichiaotsou changed the title feat: add vesting account handling on v2/cosmos/stargate feat: add vesting account handling Oct 19, 2021
@huichiaotsou huichiaotsou marked this pull request as ready for review October 19, 2021 09:58
Copy link
Contributor

@MonikaCat MonikaCat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, LGTM

@MonikaCat MonikaCat requested a review from RiccardoM October 19, 2021 13:35
database/auth.go Outdated Show resolved Hide resolved
database/auth.go Outdated Show resolved Hide resolved
database/auth.go Outdated Show resolved Hide resolved
database/auth.go Outdated Show resolved Hide resolved
database/schema/01-auth.sql Outdated Show resolved Hide resolved
types/auth.go Outdated Show resolved Hide resolved
database/types/auth.go Outdated Show resolved Hide resolved
database/auth.go Outdated Show resolved Hide resolved
database/auth.go Outdated Show resolved Hide resolved
database/auth.go Outdated Show resolved Hide resolved
modules/auth/handle_genesis.go Show resolved Hide resolved
modules/auth/handle_genesis.go Outdated Show resolved Hide resolved
modules/auth/handle_genesis.go Outdated Show resolved Hide resolved
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK. Can I have a tACK from @MonikaCat please?

database/auth.go Outdated Show resolved Hide resolved
@MonikaCat
Copy link
Contributor

tACK, LGTM

@RiccardoM RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Oct 20, 2021
@mergify mergify bot merged commit f36063f into v2/cosmos/stargate Oct 20, 2021
@mergify mergify bot deleted the v2/aaron/vesting_account branch October 20, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once all prerequisites pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vesting account data
7 participants