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

ibc-go v5: cosmos-sdk 46 #1653

Merged
merged 88 commits into from
Aug 1, 2022
Merged

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jul 5, 2022

Description

Cosmos sdk v46 changes to ibc-go


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

@faddat faddat changed the title begin changes needed for "real 46" cosmos sdk 46 upgrade Jul 5, 2022
testing/simapp/app.go Fixed Show resolved Hide resolved
@nghuyenthevinh2000
Copy link
Contributor

can anyone approve this workflow please

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022

Codecov Report

Merging #1653 (171a81e) into main (6b4aa9a) will increase coverage by 0.00%.
The diff coverage is 64.70%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1653   +/-   ##
=======================================
  Coverage   79.91%   79.92%           
=======================================
  Files         166      166           
  Lines       12434    12432    -2     
=======================================
- Hits         9937     9936    -1     
+ Misses       2030     2028    -2     
- Partials      467      468    +1     
Impacted Files Coverage Δ
...27-interchain-accounts/controller/keeper/keeper.go 94.73% <ø> (ø)
.../apps/27-interchain-accounts/host/keeper/keeper.go 83.33% <ø> (ø)
modules/apps/27-interchain-accounts/module.go 55.55% <ø> (+0.67%) ⬆️
modules/apps/29-fee/keeper/keeper.go 92.48% <ø> (ø)
modules/apps/29-fee/module.go 54.54% <ø> (+0.97%) ⬆️
modules/apps/transfer/keeper/keeper.go 91.17% <ø> (ø)
modules/apps/transfer/keeper/relay.go 88.11% <0.00%> (ø)
modules/apps/transfer/module.go 57.14% <ø> (+0.89%) ⬆️
modules/apps/transfer/types/coin.go 0.00% <0.00%> (ø)
modules/core/02-client/proposal_handler.go 77.77% <ø> (ø)
... and 10 more

@faddat faddat mentioned this pull request Jul 5, 2022
19 tasks
@crodriguezvega
Copy link
Contributor

I opened this PR, which, I think, fixes make proto-all. Please review.

@faddat
Copy link
Contributor Author

faddat commented Jul 30, 2022

I'll test it now, on a machine where it didn't work before.

@faddat
Copy link
Contributor Author

faddat commented Jul 30, 2022

Hey, I've merged your PR, and I'm now going to fix another issue in the Makefile.

@faddat
Copy link
Contributor Author

faddat commented Jul 30, 2022

All set -- so, running:

make proto-update-deps
make proto-all

in sequence now works for me perfectly.

The change was to make sure that proofs.proto ended up in the root, how buf expected it.

@vuong177 @nghuyenthevinh2000 @catShaark any additional thoughts on this one?

Eg, final cleanups and the like?

@crodriguezvega
Copy link
Contributor

@faddat When I run make proto-update-deps on my machine I get this error:

sed: 1: "third_party/proto/proof ...": undefined label 'hird_party/proto/proofs.proto'
make: *** [proto-update-deps] Error 1

Does anybody experience the same error?

Also, do you mind to run make proto-swagger-gen and push the changes in the docs/client/swagger-ui/swagger.yaml. There are some changes caused by the update of the cosmos proto files. Thanks!

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

happy to

that looks like I made a typo maybe?

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

Just tried update-deps

works fine on my mac and in gitpod

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

sed: 1: "third_party/proto/proof ...": undefined label 'hird_party/proto/proofs.proto'
make: *** [proto-update-deps] Error 1

if you got that somewhere... its gotta be somewhere right?

I am going to search the code...

Update: it's really odd:

  1. I cannot reproduce that issue
  2. I can't find any such typo
CONFIO_TYPES        = third_party/proto

proto-update-deps:
	@mkdir -p $(GOGO_PROTO_TYPES)
	@curl -sSL $(GOGO_PROTO_URL)/gogoproto/gogo.proto > $(GOGO_PROTO_TYPES)/gogo.proto

	@mkdir -p $(SDK_QUERY)
	@curl -sSL $(SDK_PROTO_URL)/base/query/v1beta1/pagination.proto > $(SDK_QUERY)/pagination.proto

	@mkdir -p $(SDK_BASE)
	@curl -sSL $(SDK_PROTO_URL)/base/v1beta1/coin.proto > $(SDK_BASE)/coin.proto

	@mkdir -p $(SDK_UPGRADE)
	@curl -sSL $(SDK_PROTO_URL)/upgrade/v1beta1/upgrade.proto > $(SDK_UPGRADE)/v1beta1/upgrade.proto

## Importing of tendermint protobuf definitions currently requires the
## use of `sed` in order to build properly with cosmos-sdk's proto file layout
## (which is the standard Buf.build FILE_LAYOUT)
## Issue link: https://github.com/tendermint/tendermint/issues/5021
	@mkdir -p $(TM_TYPES)
	@curl -sSL $(TM_URL)/types/types.proto > $(TM_TYPES)/types.proto
	@curl -sSL $(TM_URL)/types/validator.proto > $(TM_TYPES)/validator.proto

	@mkdir -p $(TM_VERSION)
	@curl -sSL $(TM_URL)/version/types.proto > $(TM_VERSION)/types.proto

	@mkdir -p $(TM_LIBS)
	@curl -sSL $(TM_URL)/libs/bits/types.proto > $(TM_LIBS)/types.proto

	@mkdir -p $(TM_CRYPTO_TYPES)
	@curl -sSL $(TM_URL)/crypto/proof.proto > $(TM_CRYPTO_TYPES)/proof.proto
	@curl -sSL $(TM_URL)/crypto/keys.proto > $(TM_CRYPTO_TYPES)/keys.proto

	@mkdir -p $(CONFIO_TYPES)
	@curl -sSL $(CONFIO_URL)/proofs.proto > $(CONFIO_TYPES)/proofs.proto

## insert go package option into proofs.proto file
## Issue link: https://github.com/confio/ics23/issues/32
	@sed -i '4ioption go_package = "github.com/confio/ics23/go";' $(CONFIO_TYPES)/proofs.proto

.PHONY: proto-all proto-gen proto-gen-any proto-swagger-gen proto-format proto-lint proto-check-breaking proto-update-deps

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

Carlos, feels mildly silly to ask these q's but ....

What os/setup do you use?
were you on the right branch?

stumped on makefile stuff

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Aug 1, 2022

Carlos, feels mildly silly to ask these q's but ....

What os/setup do you use? were you on the right branch?

stumped on makefile stuff

Hi @faddat. Thanks for checking!

I am on Big Sur and I was on the right branch. I talked to @damiannolan and he also had the same problem on his machine, but I have tried now to run make proto-update-deps on main and I get the same error, so I think it has nothing to do with your branch; it's probably something to do with my machine/setup.

@colin-axner
Copy link
Contributor

The issue @crodriguezvega is facing is a mac issue with sed. Specifically this line. We should help get this issue closed so that we can remove dependency on sed

make proto-update-deps works on my machine 👍 Also using v0.7.1 works, but results in no diffs right now

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Followup ACK. Excellent work @faddat and everyone else who contributed! Thanks for the speedy response time and applying all our suggestions

Looking forward to seeing this in a final release 🚀 🎉

testing/simapp/app.go Fixed Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

If it appeases it, let's do it.

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Awesome work! And many thanks!

It would be nice to update the CONFIO_URL to point to the v0.7.1 release version.
Lets not block on the mac os sed issue, we can address it later or more importantly address the root cause from cosmos/ics23#32

LGTM! 🚀

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Awesome, @faddat! Thanks so much for this work; this has been a fun ride! 🤠

@crodriguezvega crodriguezvega merged commit 0410963 into cosmos:main Aug 1, 2022
ulbqb added a commit to ulbqb/ibc-go that referenced this pull request Jul 26, 2023
ulbqb added a commit to ulbqb/ibc-go that referenced this pull request Jul 26, 2023
ulbqb added a commit to ulbqb/ibc-go that referenced this pull request Jul 27, 2023
ulbqb added a commit to ulbqb/ibc-go that referenced this pull request Jul 27, 2023
@ulbqb ulbqb mentioned this pull request Jul 27, 2023
5 tasks
ulbqb added a commit to Finschia/ibc-go that referenced this pull request Jul 31, 2023
* Backport part of cosmos#1653

* Replace with finschia-sdk

* Fix proto files

* Fix for finschia

* Backport part of cosmos#1653

* Fix wrong test

* Update to go 1.20

* Disable markdown-link-check

* Add NOTICE

* Add CHANGELOG for finschia
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.