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

Add admin field to base vesting account #220

Merged
merged 5 commits into from
Apr 20, 2023
Merged

Add admin field to base vesting account #220

merged 5 commits into from
Apr 20, 2023

Conversation

udpatil
Copy link
Contributor

@udpatil udpatil commented Apr 18, 2023

Describe your changes and provide context

This allows for configuring vesting accounts with an admin
This also adds the cancelled_time field to indicate when a vesting account vesting progress is cancelled

Testing performed to validate your change

Added unit tests and tested with local sei

Copy link
Contributor

@philipsu522 philipsu522 left a comment

Choose a reason for hiding this comment

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

is a store migration needed?

@@ -44,7 +45,7 @@ func NewMsgCreateVestingAccountCmd() *cobra.Command {
account can either be a delayed or continuous vesting account, which is determined
by the '--delayed' flag. All vesting accouts created will have their start time
set by the committed block's time. The end_time must be provided as a UNIX epoch
timestamp.`,
timestamp. You can also optionally configure the 'admin' field using the flag '--admin {addr}. This admin will be able to perform some administrative actions on the vesting account if set.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the vesting submodule doesn't have a query handler, is there a way to query or display the account admin from the CLI/RPC endpoint or add it a query handler or display it in seid q auth account(s)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's returned when querying the actual account seid q auth account $addr

@udpatil
Copy link
Contributor Author

udpatil commented Apr 19, 2023

is a store migration needed?

yeah good point, I'll add one in

@udpatil
Copy link
Contributor Author

udpatil commented Apr 20, 2023

actually, @psu It seems like maybe we don't need a store migration, since when we query accounts created prior to the vesting account change, they show admin as "" which is correct

@udpatil udpatil merged commit 70badca into main Apr 20, 2023
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 this pull request may close these issues.

3 participants