33 46

[faddat]

Description

I'd like to get this into a tagged release form after review

This is a notional-umee collaboration.

On today's cosmos SDK call there was some lamentation of the lack of 46 support, and Robert pointed out something pretty important to me: the v4/main I am building against in #1653 isn't yet stable and that could lead to yet more unplesantry.

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• 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
• Review Codecov Report in the comment section below once CI passes

[charleenfei]

thanks for this! we can get the formatting PR in first to reduce the number of file changes and make it easier to review.

[faddat]

Status: don't think that tests pass, but can't exactly cherry-pick @thevinhnguyen2000's earlier work from the other v4 46 pr

[faddat]

Just doing the test cleanup now, it is something with TestRandomizedGenesis...

[crodriguezvega]

Hi @faddat, thanks for the quick action on my request. Unfortunately I have made a bad judgement and, as I result, we are not going to need this PR and also this one. First of all, my apologies: I wrongly thought that we could release the upgrade to 0.46 as a minor version bump, but that is not going to be possible. There are some changes that break our Go API and therefore according to our process we should release those changes as a new major version. Apologies again!

So after discussing with the team we are going to follow this plan for the upgrade of 0.46:

• There will be no branch or release based off v3 with the upgrade to SDK 0.46. We would not be able to release this officially, since there are API breaking changes and we would need to bump the major version, and v4 is already going to be the released for fee middleware (which still uses SDK 0.45). If we just keep the upgrade as a branch, we are afraid that other teams might use this branch and then, what was initially thought as a temporary solution to unblock the gaia team to make progress with the rho upgrade, will stick around and will be difficult to get rid of.
• So we would like to release the upgrade to SDK 0.46 as v5. You already have this PR open, so I think we can focus our efforts on that one. We can merge that one to main and then cut v5.

How does this sound to you?

Sorry again that we will discard the work that you have done on those PRs on v3.

[faddat]

Sure!

The first time I read through this I didn't really realize that you were proposing a very actionable solution. We are very happy to take the four, and make it V5. Also, that provides us with a standard. So if you make the branch I will change where the PR lands and get to work on it. I propose that V5 merely be v4 with 46.

wdyt?

Also don't feel bad, I got into this knowing that there would be challenges and the code is never wasted. now I know how to write it.

[robert-zaremba]

@faddat how is this different from https://github.com/umee-network/ibc-go/tree/release/v3.1-sdk-v0.46? I see lot of commits here. I think easiest will be to:

• have working version based on v3.1.0
• once we test that it works, we merge into other branch (eg, release/v4.x or release v5...)

[robert-zaremba]

I see that you have lot of "panic: validator set is empty after InitGenesis" , I have "test panicked: cannot initialize IBC keeper: empty upgrade keeper" - did you go over the latter panic?

[crodriguezvega]

I think it's better if we focus our efforts in the branch that @faddat already has for upgrading to 0.46 based off main. Main is basically v4 at the moment, so when the branch with the upgrade is ready, then we can merge to main, and then we create release/v5.0.x.

I think I would just close the PRs that are trying to upgrade to 0.46 based on v3, since that work will never see the light of day because there are API breaking changes and we cannot release them in the v3 line.

The first time I read through this I didn't really realize that you were proposing a very actionable solution. We are very happy to take the four, and make it V5. Also, that provides us with a standard. So if you make the branch I will change where the PR lands and get to work on it. I propose that V5 merely be v4 with 46.

Thanks, @faddat! There is no need to create any release branch yet. If you work on a branch based off main, then that's fine for now. It's also better if the branch that you create is the one that gets merged to main at the end, because then you get the GitHub contributions.

@faddat, could you please update your real-46-branch with the latest changes from main? There might be some conflicts that need to be solved.

[faddat]

ok let's take the points in order:

1. @robert-zaremba - yeah those issues are fixed in the real-46 branch. Vinh fixed them and you can ask him about it in Slack. You aren't wrong to wish to pursue a 3.x edition and if we got it to a state where it passes all tests, then I think that it'd be good for Umee, and frankly good for some of our purposes as well. But it lacks the NFT support that 46 can bring. Maybe the NFT support can be merged to 3.x, I am not sure. That branch is here:

• feat: implement ics-721-nft-transfer #1510

2. How can we get this merged?

• 🧹 Linting #1418

I would like it in 4.x and 5x.

3. Re: 5.x -- So, we'd need to write it regardless (Notional). We wouldn't have any choice because we've chosen to use cosmos-sdk v46.

For 5.x, due to timeline constraints, I would like to include only certain pieces of what I consider to be known-good work:

• v4.x itself
• changes for sdk 46
• feat: implement ics-721-nft-transfer #1510

I don't think that it should contain any other scope, so that we can get it out in a timely manner.

4. I think it might be a good time to discuss some kind of a commercial arrangement between Notional and IG on this stuff. We've written 46 support since February. We'll be doing it again regardless, out of need.

5. All we're really discussing here is this:

• Integration testing between major cosmos repositories cosmos-sdk#12228

[faddat]

• V5.0.x nft bianjieai/ibc-go#1

[robert-zaremba]

@crodriguezvega -- question is about the timeline. v3.1 is stable. We need migration to 0.46 ASAP.
So, if you don't plan any official release of ibc-go with 0.46 anytime soon (~few weeks), then I would appreciate, if you can check the migration approach we did for v3.1. I didn't fix all the tests though.

[faddat]

@robert-zaremba I agree.

The tests are the hardest part though. We (notional) may have a branch that already has fixed those tests. Is there a pr you can post for reference?

[robert-zaremba]

@faddat no, I didn't make a PR of my fork (the one I shared with you last week): https://github.com/umee-network/ibc-go/tree/release/v3.1.x

[robert-zaremba]

I'm not sure what's the best way to move forward. Carlos wants to do it based on v4.0 release. I don't know what's the timeline for it.

[faddat]

@robert-zaremba I think we are happy to get both working, so that there are options. Like you, we'd like to be able to put 46 into action rapidly.

[charleenfei]

I'm not sure what's the best way to move forward. Carlos wants to do it based on v4.0 release. I don't know what's the timeline for it.

Hey @robert-zaremba the v4.0 release candidate first release candidate was cut at the beginning of the month, we will likely release the rc1 this week and final next week! if you havent already subscribed to our release/security issue notification telegram, you can get the first notification there about upcoming releases! we will strive to also be more informative wrt to the release schedule in the future :) https://t.me/ibc_is_expansive

[crodriguezvega]

To avoid confusion I think it is best to close this PR, since we cannot merge these changes in v3. They are API breaking and it would go against our standards to release API breaking changes in a minor version bump. I will also delete the branch release/v3.3.x since that will not be needed for the time being. Is that ok? @faddat @robert-zaremba

As @charleenfei said above we almost have everything needed for v4 merged in main. Hopefully early next week we can merge to main the PR of @faddat with the upgrade to 0.46 and we can already create a fist tag that teams can start using.

[robert-zaremba]

It would be amazing to start official 0.46 integration next week.

[crodriguezvega]

It would be amazing to start official 0.46 integration next week.

We'll do our best!

[robert-zaremba]

For the updates, we are going to start testing our v3.1 fork with Cosmos SDK 0.46. https://github.com/umee-network/ibc-go/tree/release/v3.1-sdk-v0.46