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 MsgCreatePermanentLockedVestingAccount #42

Merged
merged 10 commits into from
Aug 17, 2021

Conversation

clevinson
Copy link
Member

@clevinson clevinson commented Aug 10, 2021

ref: regen-network/regen-ledger#188

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@clevinson
Copy link
Member Author

I was reviewing regen-network/regen-ledger#220 to try and make sure I covered everything necessary for amino-json signing. I would appreciate a note if I missed anything. @blushi ?

@@ -53,6 +53,7 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
registry.RegisterImplementations(
(*sdk.Msg)(nil),
&MsgCreateVestingAccount{},
&MsgCreatePermanentLockedAccount{},
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't end up having to add MsgCreatePermanentLockedAccount to the RegisterLegacyAminoCodec block up above (line 14). I'm not totally sure why... but I managed to test on my Ledger device with simapp, and I successfully used the CLI for creating & signing new permanent locked accounts.

@clevinson clevinson changed the title add MsgCreatePermanentLockedVestingAccount !feat: add MsgCreatePermanentLockedVestingAccount Aug 10, 2021
@clevinson clevinson changed the title !feat: add MsgCreatePermanentLockedVestingAccount feat!: add MsgCreatePermanentLockedVestingAccount Aug 10, 2021
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

tACK. Tested with CLI and ledger device and compared with existing create vesting account message and pending create periodic vesting account message.

Should this be documented somewhere? Should we have a running changelog for patches we implement on the fork?

@clevinson
Copy link
Member Author

I think changeling at minimum is a good idea. Outside of that, personally I think CLI with clear comments / self documenting is probably good enough. I'll add the changelog though.

Afaiu, there isn't any expectation that anybody is making use of this feature aside from Regen Foundation, for the purpose of instantiating CSDAO prototypes.

@clevinson
Copy link
Member Author

Turns out the error I was receiving when trying to add the Msg type name for amino-json support was that the type name was too long and broke Ledger Nano support...

see regen-network/regen-ledger#468 for more details. Anyway-- it should all be good now!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Copy link

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

looks good - pre-approving with some smol nits

x/auth/vesting/types/msgs.go Outdated Show resolved Hide resolved
x/auth/vesting/types/msgs.go Outdated Show resolved Hide resolved
clevinson and others added 3 commits August 16, 2021 15:16
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
@clevinson clevinson merged commit 30db931 into v0.43.x-patches Aug 17, 2021
@amaury1093 amaury1093 deleted the clevinson/add-permanent-locked-acct-msg branch August 17, 2021 08:39
@amaury1093
Copy link

Nice work, would really like to see this PR in the SDK at some point too!


baseAcc, ok := baseAccountI.(*authtypes.BaseAccount)
if !ok {
return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid account type; expected: BaseAccount, got: %T", baseAccountI)

Choose a reason for hiding this comment

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

This can be sdkerrors.ErrInvalidRequest,Wrapf.... which simplifies the code

likhita-809 pushed a commit that referenced this pull request Sep 16, 2021
…ked-acct-msg

feat!: add MsgCreatePermanentLockedVestingAccount
likhita-809 pushed a commit that referenced this pull request Sep 16, 2021
…ked-acct-msg

feat!: add MsgCreatePermanentLockedVestingAccount
amaury1093 pushed a commit that referenced this pull request Sep 16, 2021
* Merge pull request #42 from regen-network/clevinson/add-permanent-locked-acct-msg

feat!: add MsgCreatePermanentLockedVestingAccount

* fix changelog

Co-authored-by: Cory <cjlevinson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants