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

All my forks dissatisfy me #685

Closed
faddat opened this issue Nov 28, 2021 · 12 comments
Closed

All my forks dissatisfy me #685

faddat opened this issue Nov 28, 2021 · 12 comments

Comments

@faddat
Copy link
Contributor

faddat commented Nov 28, 2021

Hi,

I am writing for advice. As you're aware, I made the cw 44.3 for Juno, based it from the provenance.io verison. Thing is, I know that if I do this right (TM) I will be able to easily merge in changes from upstream, and that is really critical to me.

So here's what I think the upgrade path should look like:

  1. Fork the master branch of this repository
  2. https://github.com/cosmos/ibc-go/blob/main/docs/migrations/ibc-migration-043.md
  3. https://github.com/cosmos/ibc-go/blob/main/docs/migrations/ibc-migration-v100.md

After that it is cleanup and restructuring. If you (CW team) have additional advice on this, I'd love it. I intend to maintain a fork that is as faithful as possible to the original code here that also uses the latest released version of the cosmos-sdk.

In the past week I've tried three different styles for this and to be honest, I'm not happy with any of them.

Cherry picking doesn't lead to clean merges, nor does rebasing.

I basically taught myself git rebase for this, but in the end I had divergent code. I want a graph that looks like:

Screen Shot 2021-11-28 at 8 09 30 PM

See how everything converges there?

That would be very nice.

If there is a strategy to get that, please do let me know.

@faddat
Copy link
Contributor Author

faddat commented Nov 29, 2021

Hey, this one feels sooooo much better:

https://github.com/notional-labs/wasmd/tree/fourth-try

@ethanfrey
Copy link
Member

@alpe , the lead maintainer, is currently busy with finalising the Tgrade binary.
The 0.44 upgrade is our top priority for wasmd, and he should have time to look into this in a week or so.

Lots of people have been pushing code to speed this up, but in the end, his feeling was that many of them were huge and almost more work to review thoroughly than to write. (A number of external PRs have had bugs he has caught).

I think the best way to move forward is to check with Alex on how he can best be supported. Having some PRs to look at showing what needs to be done is good. Maybe he could benefit from a chat explaining the changes high level? From a review of the final branch he makes? From testing of the result?

I do appreciate that Juno has declared their plans to upgrade on Dec 15th but there was no request from us as to when we can promise this, and I am unsure any 0.44 upgrade will be merged by then, and highly doubtful if it will be fully tested and a 1.0-beta tagged by then.

CosmWasm has long suffered from many chains pushing aggressive release timelines without checking in with us or fully supporting us. This does not make us go faster, and rather distracts us and often makes us slower. (Not signalling out Juno here, it is common)

@ethanfrey
Copy link
Member

I think the key decision, and one that will possibly prevent alignment with mainline code, is whether contracts have 20 byte or 32 byte addresses. If this is the same in both forks, you might just be able to jump from one to the other without requiring a complicated migration (but a hard fork/upgrade)

@faddat
Copy link
Contributor Author

faddat commented Dec 1, 2021

I'm going with 20, and I think that's what 42 uses, correct?

Also, very explicitly, I do not want to push on Tgrade / confio for time/resources, but instead contribute a faithful upgrade. I totally understand what you mean about the difficulty of reviewing something like this, and one of the reasons that I want a faithful upgrade is so that we can take on bugs together.

Long-run, I am interested in contributing test suites, ARM support, and more.

So, I will continue to make PR's -- I am closing #683 because it is frankly dissatisfactory -- in favor of my branch fourth-try which:

  • forks directly from wasmd master
  • uses the ibc-go upgrade guidelines
  • follows gaia v6 wherever possible (eg: root.go)
  • has a focus on accuracy and rigorous testing

I'm going to try and keep the whole thing easy-to-merge with the tip of wasmd, so that we can continue to collaborate.

Thanks for your work on wasmd guys.

@ethanfrey
Copy link
Member

I'm going with 20, and I think that's what 42 uses, correct?

That is what 42 uses.
Other 44 forks have used 32 byte ones, which was also recommended by SDK devs.

I think this needs to be agreed, mainly on the people who already have 44 ports on mainnet, so we can keep them all aligned.

@ethanfrey
Copy link
Member

On Uni testnet, contracts are 32 bytes long
(Public key "external accounts" are 20 bytes everywhere in both 0.42 and 0.44, it affects module accounts)

@iramiller
Copy link
Contributor

We (Provenance) would have used 32 (in fact that was how we originally did it on our fork) but at the time this was incompatible with IBC. The cosmoshub at the time did not endorse 0.43/0.44 so we decided we shouldn’t wait for the IBC resolution which the Cosmoshub wouldn’t support anyway.

We expect to migrate to 32 when everything else eventually caught up in a future release.

@sunnya97
Copy link
Contributor

sunnya97 commented Dec 2, 2021

I think we (osmosis) would have a preference for 32 byte. @ValarDragon has concerns about 20 byte address collision resistance.

@JakeHartnell
Copy link

I'm going with 20, and I think that's what 42 uses, correct?

That is what 42 uses. Other 44 forks have used 32 byte ones, which was also recommended by SDK devs.

I think this needs to be agreed, mainly on the people who already have 44 ports on mainnet, so we can keep them all aligned.

We'll use whatever the consensus here is. Currently using 32, but could easily switch.

@iramiller
Copy link
Contributor

I don't think there is a strong reason not to go with 32 (speaking as someone on 20 right now). When I discussed this internally it sounded like there wasn't any reason the 20 byte based addresses would stop working but of course any new loaded code or instantiation would be of the longer form.

We (Provenance) are hoping to see a standard release from cosmwasm with full 0.44+ support including these longer addresses which we already added support for in the rest of our modules.

We expect to have to do some minor work to move off of our fork to this main line and are fine with that.

@faddat
Copy link
Contributor Author

faddat commented Dec 4, 2021

Ok, 32 it is and thanks to everyone for the discussion here. I'll update my things. I also think (gasp) that I might be missing something here. Replacing the sdk.addrlen's is a little.... vexing.

I'll maybe post some questions here.

@faddat
Copy link
Contributor Author

faddat commented Dec 8, 2021

image

There. Much better!

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

No branches or pull requests

5 participants