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

feat(app)!: cosmos SDK v0.46 upgrade #1244

Merged
merged 38 commits into from
Aug 3, 2022
Merged

Conversation

aleem1314
Copy link
Contributor

@aleem1314 aleem1314 commented Jul 7, 2022

Description

Closes: #857


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@aleem1314
Copy link
Contributor Author

../../../../pkg/mod/github.com/regen-network/regen-ledger/orm@v1.0.0-beta1/table.go:19:18: undefined: types.StoreKey
../../../../pkg/mod/github.com/regen-network/regen-ledger/orm@v1.0.0-beta1/table.go:56:38: undefined: types.StoreKey
../../../../pkg/mod/github.com/regen-network/regen-ledger/orm@v1.0.0-beta1/table.go:97:18: undefined: types.StoreKey
../../../../pkg/mod/github.com/regen-network/regen-ledger/orm@v1.0.0-beta1/orm.go:32:18: undefined: types.StoreKey
../../../../pkg/mod/github.com/regen-network/regen-ledger/orm@v1.0.0-beta1/orm.go:122:17: undefined: types.StoreKey
../../../../pkg/mod/github.com/regen-network/regen-ledger/orm@v1.0.0-beta1/orm.go:139:40: undefined: types.StoreKey

Looks like we are still using regen/orm in ecocredit module.I raised a PR #1257 to update regen/orm to use SDK v0.46. I'll update this PR once we tag another regen/orm release.

@ryanchristo
Copy link
Member

Looks like we are still using regen/orm in ecocredit module.I raised a PR #1257 to update regen/orm to use SDK v0.46. I'll update this PR once we tag another regen/orm release.

We should avoid adding back in and updating regen-ledger/orm and fully migrate to cosmos-sdk/orm. Any areas we are still using regen-ledger/orm should be updated to the latest cosmos-sdk/orm.

https://github.com/cosmos/cosmos-sdk/releases/tag/orm%2Fv1.0.0-alpha.12

@ryanchristo
Copy link
Member

If v3 ecocredit migrations are a blocker because of the orm module dependency on sdk version v0.45, I would say uncomment or remove the migrations for now and maybe we can further discuss on our next architecture call. The extra work to support v3 to v5 or later seems unnecessary but it is the right idea.

I think given we are not aware of any other chains running v3, our ecocredit api was tagged v1alpha1, and sdk versioning is the main issue here, we don't need to go through the extra trouble to support upgrades directly from v3 to v5 or later.

go.mod Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #1244 (9aaa8d3) into master (682e4de) will decrease coverage by 0.23%.
The diff coverage is 65.29%.

❗ Current head 9aaa8d3 differs from pull request most recent head fafe166. Consider uploading reports for the commit fafe166 to get more accurate results

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
- Coverage   77.45%   77.22%   -0.24%     
==========================================
  Files         221      213       -8     
  Lines       16120    15393     -727     
==========================================
- Hits        12486    11887     -599     
+ Misses       2620     2597      -23     
+ Partials     1014      909     -105     
Flag Coverage Δ
experimental-codecov 77.22% <65.29%> (-0.34%) ⬇️
stable-codecov 77.22% <65.29%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/export.go 10.20% <0.00%> (+0.40%) ⬆️
app/regen/cmd/genaccounts.go 15.15% <0.00%> (-0.98%) ⬇️
app/regen/cmd/testnet.go 16.79% <0.00%> (ø)
types/module/server/root_module_key.go 11.76% <ø> (+5.88%) ⬆️
x/data/client/testsuite/grpc.go 100.00% <ø> (ø)
x/data/msg_anchor.go 80.00% <ø> (ø)
x/data/msg_attest.go 60.00% <ø> (ø)
x/data/msg_define_resolver.go 80.00% <ø> (ø)
x/data/msg_register_resolver.go 81.81% <ø> (ø)
x/data/server/server.go 71.42% <ø> (ø)
... and 61 more

@aleem1314 aleem1314 marked this pull request as ready for review August 1, 2022 06:50
@aleem1314
Copy link
Contributor Author

aleem1314 commented Aug 1, 2022

Marking as R4R.

These two updates pending, maybe we can add them in follow-up PRs.

  1. #10977 Now every cosmos message protobuf definition must be extended with a cosmos.msg.v1.signer option to signal the signer fields in a language agnostic way.
  2. Should add support for Transaction Tips and SIGN_MODE_DIRECT_AUX?

For first update, I'm receiving following error when I add option (cosmos.msg.v1.signer) = "curator"; annotation to txns.

regen/ecocredit/v1/tx.proto:284:10:message regen.ecocredit.v1.MsgSend: unknown extension cosmos.msg.v1.signer
regen/ecocredit/v1/tx.proto:329:10:message regen.ecocredit.v1.MsgRetire: unknown extension cosmos.msg.v1.signer
regen/ecocredit/v1/tx.proto:352:10:message regen.ecocredit.v1.MsgCancel: unknown extension cosmos.msg.v1.signer
regen/ecocredit/v1/tx.proto:370:10:message regen.ecocredit.v1.MsgUpdateClassAdmin: unknown extension cosmos.msg.v1.signer

I'm using the same docker image as in 0.46 (v0.7) and buf.gen.gogo.yaml is the same.

@technicallyty
Copy link
Contributor

option (cosmos.msg.v1.signer)

only thing i could possibly think of is a missing import? import "cosmos/msg/v1/msg.proto"; i know robert had a similar issue in the cosmos core dev discord: https://discord.com/channels/669268347736686612/984437601857568768/1002138288250957834

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Nice work! Looks good to me aside from a few minor changes, in particular the passing of the account prefix rather than the bond denom to the account keeper.

app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/encoding.go Outdated Show resolved Hide resolved
scripts/test_cover.sh Outdated Show resolved Hide resolved
x/ecocredit/server/server_test.go Outdated Show resolved Hide resolved
x/ecocredit/server/tests/utils.go Show resolved Hide resolved
x/data/msg_anchor.go Show resolved Hide resolved
aleem1314 and others added 5 commits August 2, 2022 14:40
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Nice work! One item that still needs to be addressed in app.go (we are still using uregen rather than the bech32 account prefix). Otherwise, looks good to me. I opened followup issues for the two items you mentioned above: #1330 and #1331.

aleem1314 and others added 2 commits August 3, 2022 14:58
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
@ryanchristo ryanchristo merged commit 425690c into master Aug 3, 2022
@ryanchristo ryanchristo deleted the aleem/857-sdk-v0.46 branch August 3, 2022 15:06
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.

Update to cosmos sdk version v0.46
3 participants