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.

Sign up for GitHub

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

Jump to bottom

Problem: cosmos-sdk 0.46 is not used #69

Merged
merged 2 commits into from
Aug 15, 2022
Merged

Problem: cosmos-sdk 0.46 is not used #69

merged 2 commits into from
Aug 15, 2022

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Jul 8, 2022 •
edited
Loading

also upgrade ibc-go to v5

@yihuang yihuang requested a review from thomas-nguy July 8, 2022 07:05
@yihuang yihuang force-pushed the ibc-v4 branch 2 times, most recently from 5901d41 to 874ce92 Compare July 8, 2022 08:00
@thomas-nguy
Copy link
Collaborator

seems to be a breaking change.

mmm lets see if we should have main branch or keep syncinc with upstream

@yihuang yihuang changed the title Problem: ibc-go v4 is not used Problem: cosmos-sdk 0.46 is not used Jul 8, 2022
@yihuang
Copy link
Collaborator Author

yihuang commented Jul 14, 2022 •
edited
Loading

opened a PR to upstream too: PeggyJV#433

thomas-nguy
thomas-nguy previously approved these changes Jul 14, 2022
View reviewed changes
Copy link
Collaborator

@thomas-nguy thomas-nguy left a comment

Choose a reason for hiding this comment

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

lgtm

@yihuang
Copy link
Collaborator Author

yihuang commented Jul 14, 2022

the only thing is it rely on a unmerged upstream PR: cosmos/ibc-go#1653

@faddat
Copy link

faddat commented Jul 14, 2022

cosmos/ibc-go#1706

@faddat
Copy link

faddat commented Jul 14, 2022 •
edited
Loading

Hi, I should give a little context here I think:

  • yes, it is an upstream unmerged pr
  • yes, probably okay to use
  • no, probably not okay to use in prod but helpful for refactoring

Since ibc v4 hasn't cut a release with either 3.x or 4.x that uses 46, yeah this has been a pain point for me since february.

I will be making another for ibc v3.1.0, that upgrades it to 46.

I have no idea when any of this will be merged, not sure if keeping in sync with cosmos-sdk release candidate versions is a priority for the ibc-go team.

@thomas-nguy it is definitely several breaking changes.

If I were you, and this doesn't ship straight into production then I would:

  • merge so that you can build with v4 and 46
  • check in frequently at that pull request
  • go get github.com/notional-labs/ibc-go@latestcommitheight frequently: that pr is going to evolve while:
    • 46 is finalized
    • v4 is finalized

thomas-nguy
thomas-nguy previously approved these changes Aug 11, 2022
View reviewed changes
Copy link
Collaborator

@thomas-nguy thomas-nguy left a comment

Choose a reason for hiding this comment

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

lgtm

thomas-nguy
thomas-nguy reviewed Aug 11, 2022
View reviewed changes
module/x/gravity/client/cli/tx.go
@@ -128,7 +128,7 @@ func CmdRequestBatchTx() *cobra.Command {
}

denom := args[0]
signer, err := sdk.AccAddressFromHex(args[1])
signer, err := sdk.AccAddressFromHexUnsafe(args[1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it unsafe?

Copy link
Collaborator Author

@yihuang yihuang Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

maybe it can't tell if it's actually an address or some random 20 bytes, anyway, cosmos-sdk renamed the function.

@tomtau tomtau mentioned this pull request Aug 11, 2022
Merged
13 tasks
@yihuang
Problem: cosmos-sdk 0.46 is not used
f24af55 
also update ibc-go to v4

cosmos-sdk reject zero gas limit

Revert "cosmos-sdk reject zero gas limit"

This reverts commit 589525b.

update ibc-go to v5 beta

fix building unit tests

fix unit tests
tomtau
tomtau approved these changes Aug 15, 2022
View reviewed changes
Copy link

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

I guess move from x/params and the new gov proposal-related changes are to be done in a separate PR?

module/go.sum
Comment on lines +425 to +426
github.com/ethereum/go-ethereum v1.10.17 h1:XEcumY+qSr1cZQaWsQs5Kck3FHB0V2RiMHPdTBJ+oT8=
github.com/ethereum/go-ethereum v1.10.17/go.mod h1:Lt5WzjM07XlXc95YzrhosmR4J9Ahd6X2wyEV2SvGhk0=
Copy link

Choose a reason for hiding this comment

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

I guess this one doesn't affect anything: GHSA-rqmg-hrg4-fm69

Copy link
Collaborator Author

@yihuang yihuang Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we can do a replace in cronos.

module/x/gravity/keeper/invariants_test.go
@@ -17,12 +18,12 @@ func TestModuleBalanceUnbatchedTxs(t *testing.T) {

ctx := input.Context
var (
mySender, _ = sdk.AccAddressFromBech32("gravity1ahx7f8wyertuus9r20284ej0asrs085ceqtfnm")
mySender, _ = sdk.AccAddressFromBech32("cosmos12luku6uxehhak02py4rcz65zu0swh7wj8a5enl")
Copy link

Choose a reason for hiding this comment

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

bech32 prefix is hardcoded?

Copy link
Collaborator Author

@yihuang yihuang Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see it customized in unit tests, so I have to change to default one to make the tests run

@yihuang
Copy link
Collaborator Author

yihuang commented Aug 15, 2022

I guess move from x/params and the new gov proposal-related changes are to be done in a separate PR?

yeah

@yihuang yihuang merged commit 48275db into crypto-org-chain:v2.0.0-cronos Aug 15, 2022
@yihuang yihuang deleted the ibc-v4 branch August 15, 2022 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomtau tomtau tomtau approved these changes

@thomas-nguy thomas-nguy Awaiting requested review from thomas-nguy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yihuang @thomas-nguy @faddat @tomtau