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

Use forked packages #16

Merged
merged 6 commits into from
Apr 26, 2024
Merged

Use forked packages #16

merged 6 commits into from
Apr 26, 2024

Conversation

marcello33
Copy link
Collaborator

@marcello33 marcello33 commented Apr 25, 2024

In this PR, we replace some cosmossdk.io registry based packages with the ones used in our fork.
Also, we replace cosmos-sdk and cometBFT with our forked versions.
As a "side-effect" the code has been changed in some places to use our own code.

I made sure that:

  • the project builds
  • the unit tests for customized modules (x/auth, x/bank, crypto, x/gov) are green
  • the CI can build the app with the updated dependencies

Notes

  • Some packages have been replaced with the same tag on our fork, i.e. cosmossdk.io/api => github.com/0xPolygon/cosmos-sdk/api v0.7.2
  • Some packages have been replaced with "latest" devel version of our fork, because we modified them (e.g. to remove the bech32 stuff) but we did not produce any new tag, i.e. cosmossdk.io/x/circuit => github.com/0xPolygon/cosmos-sdk/x/circuit v0.1.1-0.20240409091007-84d9b34a8681
  • Some packages have been replaced with our own forked tags (this only applies to cosmos-sdk itself and cometBFT), i.e. github.com/cometbft/cometbft => github.com/0xPolygon/cometbft v0.1.0-beta and github.com/cosmos/cosmos-sdk => github.com/0xPolygon/cosmos-sdk v0.1.0-beta-polygon

If approved and merged, a new release will be created and used in heimdall-v2, where a subset of such replace directives will be added.

Copy link

@marcello33 your pull request is missing a changelog!

go.mod Outdated
github.com/cometbft/cometbft => github.com/0xPolygon/cometbft v0.1.0-beta
github.com/cosmos/cosmos-sdk => github.com/0xPolygon/cosmos-sdk v0.50.3-0.20240409090657-2e910ae577a9
github.com/cosmos/cosmos-sdk => github.com/0xPolygon/cosmos-sdk v0.1.0-beta-polygon
Copy link
Member

Choose a reason for hiding this comment

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

Everytime we have a new release, we will need to update this, right? Wondering if there's a different way this can be done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, not sure about alternatives

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check my comment above about the relative path. It can be used during development. Then, when you release a version of the fork, you just need to remember to come here and set the official version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, actually, are you sure you need this line in particular. What breaks if you delete it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd need to double check this, but iirc, the following happens if I remove that replace.
While importing cosmos-sdk as a dependency in heimdall-v2 I get parsing go.mod module declares its path as "github.com/cosmos/cosmos-sdk" but was required as "github.com/0xPolygon/cosmos-sdk"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyway using the relative path now, but I confirm that was the issue
Addressed here 1554aba

Copy link
Collaborator

Choose a reason for hiding this comment

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

The relative path is good, I think, since it removes the hassle of having to be changing it all the time while in heavy development.

But, I'm still intrigued by the error you're getting. For context, what motivated my comment initially was the fact of seeing that the very first version of CometBFT (when we forked from Tendermint Core) declares itself as github.com/tendermint/tendermint see here , and so, there is no require of tendermint/tendermint anywhere in that go.mod.

I also checked Celestia's fork, which is now a fork of CometBFT. Same situation.

So, since you declare your repo in the same way (module github.com/cosmos/cosmos-sdk), I thought you wouldn't need to include it in the require section.

Just to be clear, I'm only referring to the line this thread is attached to (the top level go.mod). For all other go.mod files, the relative path is the best approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @sergio-mena
Just created the release, will re-check this while importing in heimdall-v2 and - in case - make the required change

Copy link
Collaborator

@sergio-mena sergio-mena Apr 26, 2024

Choose a reason for hiding this comment

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

Resolved (AFAIAC) -- notice that @pratikspatil024 opened this

@marcello33 marcello33 marked this pull request as draft April 25, 2024 11:21
@marcello33 marcello33 changed the title use forked sdk.io packages [WIP] use forked sdk.io packages Apr 25, 2024
@marcello33 marcello33 changed the title [WIP] use forked sdk.io packages use forked packages Apr 25, 2024
@marcello33 marcello33 changed the title use forked packages Use forked packages Apr 25, 2024
@marcello33 marcello33 marked this pull request as ready for review April 25, 2024 13:23
go.mod Outdated Show resolved Hide resolved
tools/confix/go.mod Outdated Show resolved Hide resolved
x/gov/abci_test.go Outdated Show resolved Hide resolved
Copy link
Member

@pratikspatil024 pratikspatil024 left a comment

Choose a reason for hiding this comment

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

Any idea where this change in keys.pulsar.go and keys.pb.go came from?

@marcello33
Copy link
Collaborator Author

Any idea where this change in keys.pulsar.go and keys.pb.go came from?

Yes, @pratikspatil024. I updated a comment once in a proto definition and most probably forgot to run make proto-all.
Fortunately, it's just a comment.

@marcello33 marcello33 merged commit 2718dda into devel Apr 26, 2024
16 of 44 checks passed
@sergio-mena
Copy link
Collaborator

LGTM 👍

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.

4 participants