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!: periodic vesting msg #9596

Merged
merged 33 commits into from
Aug 25, 2021
Merged

Conversation

zmanian
Copy link
Member

@zmanian zmanian commented Jun 28, 2021

Closes: #9595

Enables creating a periodic vesting account on a running chain.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

x/auth/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/auth/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/auth/vesting/client/cli/tx.go Show resolved Hide resolved
@clevinson clevinson self-assigned this Jun 28, 2021
zmanian and others added 2 commits June 28, 2021 11:00
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Copy link
Contributor

@clevinson clevinson left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

A few change requests:

  • Test needs updating (the one test added doesn't compile)
  • We should remove the --delayed flag from this command as it doesn't apply.

Other than that, some comment / naming suggestions, and I wanted to raise a question abt allowing period == 0. Don't have a super strong opinion on this, but potentially worth reconsidering if there is a valid use case for that.

x/auth/vesting/client/cli/tx.go Show resolved Hide resolved
x/auth/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/auth/vesting/msg_server.go Outdated Show resolved Hide resolved
x/auth/vesting/types/msgs.go Outdated Show resolved Hide resolved
x/auth/vesting/types/msgs.go Outdated Show resolved Hide resolved
x/auth/vesting/types/msgs.go Outdated Show resolved Hide resolved
x/auth/vesting/handler_test.go Outdated Show resolved Hide resolved
x/auth/vesting/handler_test.go Outdated Show resolved Hide resolved
@amaury1093 amaury1093 self-assigned this Jul 1, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
proto/cosmos/vesting/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/vesting/v1beta1/tx.proto Show resolved Hide resolved
x/auth/vesting/client/cli/tx.go Show resolved Hide resolved
zmanian and others added 2 commits July 6, 2021 10:16
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Cory <cjlevinson@gmail.com>
@orijbot
Copy link

orijbot commented Jul 16, 2021

Visit https://dashboard.github.orijtech.com?pr=9596&repo=cosmos%2Fcosmos-sdk to see benchmark details.

zmanian and others added 2 commits July 15, 2021 20:15
actually committing the account state to periodic vesting was missing.
Co-authored-by: Cory <cjlevinson@gmail.com>
@robert-zaremba
Copy link
Collaborator

This PR is almost done and close to be merged. @zmanian - could you do remaining updates?

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
@zmanian
Copy link
Member Author

zmanian commented Aug 25, 2021

lgtm, but we should really add tests.

@zmanian if you don't have bandwidth to write tests, I'm okay to create a follow-up issue and assign to someone on the sdk team

Yeah the vesting module generally doesn't have a enough tests. I would appreciate help writing them

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 25, 2021
@mergify mergify bot merged commit 147d798 into master Aug 25, 2021
@mergify mergify bot deleted the zaki-upstream-periodic-vesting-msg branch August 25, 2021 10:03
@amaury1093
Copy link
Contributor

follow-up: #10005

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2021

Sorry but I didn't understand the command. Please consult the commands documentation 📚.

Hey, I reacted but my real name is @Mergifyio

mergify bot pushed a commit that referenced this pull request Aug 25, 2021
…#10007)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

We merged #9596, but we forgot to add proto comments for one message.

Then, proto lint CI fails on unrelated PRs, e.g. https://github.com/cosmos/cosmos-sdk/pull/9759/checks?check_run_id=3421076482#step:4:24. This fixes it.

Also related to #9978

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
dongsam pushed a commit to cosmosquad-labs/cosmos-sdk that referenced this pull request Feb 22, 2022
dongsam pushed a commit to cosmosquad-labs/cosmos-sdk that referenced this pull request Mar 11, 2022
dongsam added a commit to cosmosquad-labs/cosmos-sdk that referenced this pull request Mar 11, 2022
* custom/vesting-acc:
  feat: periodic vesting msg, cherry-pick 147d798 cosmos#9596
dongsam pushed a commit to cosmosquad-labs/cosmos-sdk that referenced this pull request Apr 22, 2022
(cherry picked from commit 5d01af4)
(cherry picked from commit aefda6b)
dongsam added a commit to cosmosquad-labs/cosmos-sdk that referenced this pull request Apr 22, 2022
…k-0.45.3

* custom/0.45.3/vesting-acc:
  feat: periodic vesting msg, cherry-pick 147d798 cosmos#9596
yun-yeo pushed a commit to terra-money/cosmos-sdk that referenced this pull request Jun 2, 2022
Closes: cosmos#9595

Enables creating a periodic vesting account on a running chain.

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

chore: Add proto comments for MsgCreatePeriodicVestingAccountResponse (cosmos#10007)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

We merged cosmos#9596, but we forgot to add proto comments for one message.

Then, proto lint CI fails on unrelated PRs, e.g. https://github.com/cosmos/cosmos-sdk/pull/9759/checks?check_run_id=3421076482#step:4:24. This fixes it.

Also related to cosmos#9978

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
jaybxyz pushed a commit to cosmosquad-labs/cosmos-sdk that referenced this pull request Oct 17, 2022
(cherry picked from commit 5d01af4)
(cherry picked from commit aefda6b)
(cherry picked from commit 82c4293)
queencre pushed a commit to crescent-network/cosmos-sdk that referenced this pull request Oct 17, 2022
(cherry picked from commit 5d01af4)
(cherry picked from commit aefda6b)
(cherry picked from commit 82c4293)
(cherry picked from commit 071fe25)
queencre pushed a commit to crescent-network/cosmos-sdk that referenced this pull request Dec 20, 2022
(cherry picked from commit 5d01af4)
(cherry picked from commit aefda6b)
(cherry picked from commit 82c4293)
(cherry picked from commit 071fe25)
(cherry picked from commit 54143fc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable creating of periodic vesting accounts on a running chain
7 participants