8000 feat: amino encoding support for the vesting module (backport #1070) by mergify[bot] · Pull Request #1076 · evmos/evmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: amino encoding support for the vesting module (backport #1070) #1076

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 16, 2022
Merged

feat: amino encoding support for the vesting module (backport #1070) #1076

merged 4 commits into from
Nov 16, 2022

Conversation

mergify[bot]
Copy link
Contributor
@mergify mergify bot commented Nov 11, 2022

This is an automatic backport of pull request #1070 done by Mergify.
Cherry-pick of c768d16 has failed:

On branch mergify/bp/release/v10.0.x/pr-1070
Your branch is up to date with 'origin/release/v10.0.x'.

You are currently cherry-picking commit c768d16.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   x/vesting/module.go
	modified:   x/vesting/types/codec.go
	modified:   x/vesting/types/msg.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   CHANGELOG.md

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@hanchon @mergify
feat: amino encoding support for the vesting module (#1070) 8000
30d0650 
* feat: amino enconding support for the vesting module

* fix: missing enconding added

* chore: update changelog

* chore: remove temp files

(cherry picked from commit c768d16)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot requested review from ramacarlucho and a team as code owners November 11, 2022 16:19
@mergify mergify bot added the conflicts label Nov 11, 2022
@mergify mergify bot requested a review from hanchon November 11, 2022 16:19
@mergify mergify bot mentioned this pull request Nov 11, 2022
Merged
12 tasks
@codecov
Copy link
codecov bot commented Nov 11, 2022 •
edited
Loading

Codecov Report

Merging #1076 (1cb1f39) into release/v10.0.x (e0ed40f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##           release/v10.0.x    #1076      +/-   ##
===================================================
+ Coverage            77.59%   77.61%   +0.01%     
===================================================
  Files                  128      128              
  Lines                 7494     7500       +6     
===================================================
+ Hits                  5815     5821       +6     
  Misses                1535     1535              
  Partials               144      144
Impacted Files Coverage Δ
x/vesting/types/codec.go 17.64% <100.00%> (+17.64%) ⬆️
x/vesting/types/msg.go 100.00% <100.00%> (ø)

@fedekunze
Copy link
Collaborator

@hanchon can you fix the conflicts when you have time?
@hanchon
Copy link
Contributor
hanchon commented Nov 14, 2022

@hanchon can you fix the conflicts when you have time?
I just fixed the changelog conflicts!

fedekunze
fedekunze approved these changes Nov 14, 2022
View reviewed changes
@fedekunze fedekunze enabled auto-merge (squash) November 14, 2022 17:36
MalteHerrmann
MalteHerrmann reviewed Nov 14, 2022
View reviewed changes
Copy link
Contributor
@MalteHerrmann MalteHerrmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The only remaining use of types.ModuleCdc in the vesting module seems to be here:
https://github.com/evmos/evmos/blob/main/x/vesting/keeper/integration_test.go#L475

Should we change this to use AminoCdc as well? In that case, I think ModuleCdc would be unused and could also be removed from the file.

P.S.: In the context of looking for the usage of the vesting codec, I also found this TODO: https://github.com/evmos/evmos/blob/main/app/ante/vesting.go#L79
@fedekunze should this be addressed now that were on sdk v0.46.4?

@fedekunze @MalteHerrmann
Apply suggestions from code review
1cb1f39 
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
@fedekunze fedekunze disabled auto-merge November 16, 2022 10:28
@fedekunze fedekunze merged commit eceafaf into release/v10.0.x Nov 16, 2022
@fedekunze fedekunze deleted the mergify/bp/release/v10.0.x/pr-1070 branch November 16, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MalteHerrmann MalteHerrmann MalteHerrmann left review comments

@fedekunze fedekunze fedekunze approved these changes

@ramacarlucho ramacarlucho Awaiting requested review from ramacarlucho

@hanchon hanchon Awaiting requested review from hanchon hanchon is a code owner automatically assigned from evmos/core-engineering

Assignees
No one assigned
Labels
conflicts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fedekunze @hanchon @MalteHerrmann
0