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

upgraded to ics v3.2.0 #1043

Merged
merged 11 commits into from
Jan 10, 2024
Merged

upgraded to ics v3.2.0 #1043

merged 11 commits into from
Jan 10, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Jan 3, 2024

Context

Upgraded ICS to v3.2. We previously used a fork of ICS that had ibctesting infra for our unit tests; however, the fork is no longer necessary as it has been moved to ibc-go v7.3.

Brief Changelog

  • Upgrade ICS to v3.2 and removed fork reference
  • Upgraded to sdk v0.47.5 and ibc v7.3.0
  • Updated ibctesting setup to use the testing app from ibc-go instead of the ics repo

@sampocs sampocs requested a review from asalzmann January 3, 2024 23:21
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jan 3, 2024
@sampocs sampocs added the v17 label Jan 3, 2024
@sampocs sampocs marked this pull request as draft January 3, 2024 23:26
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

this change lgtm - do you know why the v3.2.0-testing-infra release shows up as being pushed by @strbrian ?

go.mod Outdated Show resolved Hide resolved
@sampocs
Copy link
Collaborator Author

sampocs commented Jan 4, 2024

this change lgtm - do you know why the v3.2.0-testing-infra release shows up as being pushed by @strbrian ?

I was wondering the same thing haha, I think it's cause I cherry picked the commit so it probably keeps the original author

@sampocs
Copy link
Collaborator Author

sampocs commented Jan 4, 2024

Also, fyi, I haven't been able to get this to build yet. Working on something else rn, but will come back to it tomorrow

@sampocs sampocs marked this pull request as ready for review January 9, 2024 21:59
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Really impressive work jumping into this and figuring it out so quickly.

The tests look to be equivalent, so I'd expect full ICS test coverage after this change.

My main takeaway is that we made some changes to how the initial validator set is passed into the consumer genesis as the stride chain's initial set.

go.mod Show resolved Hide resolved
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

assuming v0.47.5-stride-distribution-fix-0 has been reviewed, this lgtm

@asalzmann
Copy link
Contributor

Screen Shot 2024-01-09 at 10 38 22 PM

review ^

@sampocs sampocs merged commit 2113b4f into main Jan 10, 2024
10 checks passed
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.

3 participants