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

init commit for bump ibc-go to v6.1.0 #1196

Merged
merged 31 commits into from Jan 12, 2023
Merged

Conversation

sontrinh16
Copy link
Contributor

@sontrinh16 sontrinh16 commented Dec 27, 2022

Description

Bump ibc-go version to v6.1.0


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.

PR review checkboxes:

  • Pass all previous test

@faddat
Copy link
Contributor

faddat commented Dec 30, 2022

Getting this merge-ready depends on merging this PR in ethermint:

evmos/ethermint#1575

@fedekunze
Copy link
Contributor

@sontrinh16 looks good, thanks! Is there anything left to address to mark it R4R?

@sontrinh16
Copy link
Contributor Author

i think i'm just wait for a ethermint release that use ibc-go v6 to run all the unit test for evmos

Copy link
Contributor

@adisaran64 adisaran64 left a comment

Choose a reason for hiding this comment

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

hey @sontrinh16 , looks great! could you take a look at this again now that ethermint has been bumped to ibc v6? looks like there are some issues with the unit tests when I ran them. If you're too busy, would be great if I could get access to the branch to finish it up for you.

@sontrinh16
Copy link
Contributor Author

hey @sontrinh16 , looks great! could you take a look at this again now that ethermint has been bumped to ibc v6? looks like there are some issues with the unit tests when I ran them. If you're too busy, would be great if I could get access to the branch to finish it up for you.

oh sweet gonna doing now, i was just fixing test for a forked ibc 6 ethermint and running on some same proto err evmos have on ethermint

@sontrinh16 sontrinh16 marked this pull request as ready for review January 7, 2023 19:34
@sontrinh16
Copy link
Contributor Author

@fedekunze, i think it's ready for review now

@sontrinh16 sontrinh16 closed this Jan 9, 2023
@sontrinh16 sontrinh16 reopened this Jan 9, 2023
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #1196 (17de501) into main (6d21104) will decrease coverage by 0.25%.
The diff coverage is 41.42%.

❗ Current head 17de501 differs from pull request most recent head 5e291bc. Consider uploading reports for the commit 5e291bc to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1196      +/-   ##
==========================================
- Coverage   76.45%   76.19%   -0.26%     
==========================================
  Files         149      149              
  Lines        8238     8280      +42     
==========================================
+ Hits         6298     6309      +11     
- Misses       1749     1779      +30     
- Partials      191      192       +1     
Impacted Files Coverage Δ
app/ante/handler_options.go 0.00% <ø> (ø)
app/upgrades/v11/upgrades.go 36.44% <0.00%> (ø)
ibc/module.go 100.00% <ø> (ø)
ibc/utils.go 100.00% <ø> (ø)
x/claims/ibc_middleware.go 15.62% <0.00%> (-5.21%) ⬇️
x/claims/keeper/hooks.go 67.00% <0.00%> (-9.14%) ⬇️
x/claims/keeper/ibc_callbacks.go 97.76% <ø> (ø)
x/claims/keeper/keeper.go 84.61% <ø> (ø)
x/claims/types/params.go 100.00% <ø> (ø)
x/erc20/ibc_middleware.go 14.28% <ø> (ø)
... and 9 more

Copy link
Contributor

@adisaran64 adisaran64 left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

The replace of Ethermint in go.mod breaks our upgrade tests (make test-upgrade), please revise. I think the reason are problems we still have on Ethermint with the recent changes to how parameters are stored.

We shouldn't merge it in this context IMO

10:20AM INF migrating module evm from version 3 to version 4
panic: parameter EVMDenom not registered

go.mod Outdated Show resolved Hide resolved
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!. I'll follow up with another PR replacing Ethermint with our own and fixing the upgrade tests.

@fedekunze fedekunze dismissed MalteHerrmann’s stale review January 12, 2023 20:37

@Vvaradinov will address the tests in a separate PR

@fedekunze fedekunze enabled auto-merge (squash) January 12, 2023 20:37
@fedekunze fedekunze merged commit a9e0a9c into evmos:main Jan 12, 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.

None yet

7 participants