Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix: avoid duplicate register proto type in evm & feemarket #1586

Merged
merged 16 commits into from
Jan 12, 2023

Conversation

mmsqe
Copy link
Contributor

@mmsqe mmsqe commented Jan 5, 2023

Closes: #XXX

Description

This PR mainly focus duplicate register proto type in evm & feemarket, also

  • fix migrating module evm from version 3 to version 4
  • fix migrating module feemarket from version 3 to version 4
  • update from version in upgrade test to v.19.x which support submit-proposal
  • keep grpc query compatible with version before migrate
  • support new ethermint.feemarket.v1.EventFeeMarket with compatible type
  • fix wrong encode with extra quote "

For contributor use:

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Copy link
Contributor Author

@mmsqe mmsqe left a comment

Choose a reason for hiding this comment

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

@Vvaradinov Evm migrate is fixed, but recent merged feemarket also get same issue.

@mmsqe mmsqe changed the title fix: avoid duplicate register proto type in evm fix: avoid duplicate register proto type in evm & feemarket Jan 9, 2023
@Vvaradinov
Copy link
Contributor

@Vvaradinov Evm migrate is fixed, but recent merged feemarket also get same issue.

Hey @mmsqe great work on this! Can you check out evmos/evmos#1219 and try to localize the legacy x/params methods to the migrations folder?

@mmsqe
Copy link
Contributor Author

mmsqe commented Jan 9, 2023

@Vvaradinov Evm migrate is fixed, but recent merged feemarket also get same issue.

Hey @mmsqe great work on this! Can you check out evmos/evmos#1219 and try to localize the legacy x/params methods to the migrations folder?

Roger that, just want to list out changes first to avoid duplicate work.

rpc/backend/blocks.go Outdated Show resolved Hide resolved
@mmsqe
Copy link
Contributor Author

mmsqe commented Jan 10, 2023

@Vvaradinov Emit eventEthereumTx in AnteHandle is different from grpc MsgServer one, need to revert #1544 to unblock the test

@Vvaradinov
Copy link
Contributor

@Vvaradinov Emit eventEthereumTx in AnteHandle is different from grpc MsgServer one, need to revert #1544 to unblock the test

I'm currently investigating this in a separate PR #1592. It could require reverting to the old EmitEvents to unblock this and future work until I make the required changes and test them.

@Vvaradinov
Copy link
Contributor

@mmsqe I've reverted the changes in re to event emitting in #1600 so this should be unblocked, let me know if you run into any other issues.

@mmsqe
Copy link
Contributor Author

mmsqe commented Jan 11, 2023

@mmsqe I've reverted the changes in re to event emitting in #1600 so this should be unblocked, let me know if you run into any other issues.

Thanks, I just revert part of handle for typed event too, but we need keep subspace for query compatible with version before migrate, otherwise need sth like #1601

@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #1586 (90fb353) into main (c147b8e) will increase coverage by 0.01%.
The diff coverage is 71.69%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1586      +/-   ##
==========================================
+ Coverage   68.31%   68.33%   +0.01%     
==========================================
  Files         110      110              
  Lines       10087    10171      +84     
==========================================
+ Hits         6891     6950      +59     
- Misses       2793     2818      +25     
  Partials      403      403              
Impacted Files Coverage Δ
x/evm/types/params.go 75.60% <0.00%> (-11.72%) ⬇️
x/feemarket/keeper/params.go 65.00% <0.00%> (-3.43%) ⬇️
x/feemarket/types/params.go 82.75% <76.47%> (-4.94%) ⬇️
x/evm/keeper/params.go 84.46% <86.36%> (+2.85%) ⬆️
x/evm/migrations/v4/migrate.go 78.57% <87.50%> (ø)
app/app.go 84.96% <100.00%> (+0.05%) ⬆️
x/evm/keeper/keeper.go 87.20% <100.00%> (+0.07%) ⬆️
x/feemarket/keeper/keeper.go 86.95% <100.00%> (+0.28%) ⬆️
x/feemarket/migrations/v4/migrate.go 83.33% <100.00%> (ø)

@mmsqe mmsqe marked this pull request as ready for review January 11, 2023 15:42
@mmsqe mmsqe requested a review from a team as a code owner January 11, 2023 15:42
@mmsqe mmsqe requested review from hanchon and adisaran64 and removed request for a team January 11, 2023 15:42
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @mmsqe.

@Vvaradinov Vvaradinov merged commit 5f7ee9b into evmos:main Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants