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

feat(ica)!: support json tx encoding for interchain accounts (backport #3796) #4006

Merged
merged 4 commits into from
Jul 6, 2023
Merged

feat(ica)!: support json tx encoding for interchain accounts (backport #3796) #4006

merged 4 commits into from
Jul 6, 2023

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Jul 4, 2023

This is an automatic backport of pull request #3796 done by Mergify.
Cherry-pick of e5b057d has failed:

On branch mergify/bp/release/v7.3.x/pr-3796
Your branch is up to date with 'origin/release/v7.3.x'.

You are currently cherry-picking commit e5b057da.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	new file:   modules/apps/27-interchain-accounts/host/keeper/export_test.go
	modified:   modules/apps/27-interchain-accounts/host/keeper/genesis_test.go
	modified:   modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
	modified:   modules/apps/27-interchain-accounts/types/metadata.go
	modified:   modules/apps/27-interchain-accounts/types/metadata_test.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   modules/apps/27-interchain-accounts/controller/keeper/keeper.go
	both modified:   modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go
	both modified:   modules/apps/27-interchain-accounts/host/client/cli/tx_test.go
	both modified:   modules/apps/27-interchain-accounts/host/ibc_module_test.go
	both modified:   modules/apps/27-interchain-accounts/host/keeper/keeper.go
	both modified:   modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
	both modified:   modules/apps/27-interchain-accounts/host/keeper/relay.go
	both modified:   modules/apps/27-interchain-accounts/host/keeper/relay_test.go
	both modified:   modules/apps/27-interchain-accounts/types/codec.go
	both modified:   modules/apps/27-interchain-accounts/types/codec_test.go

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@srdtrk @mergify
feat(ica)!: support json tx encoding for interchain accounts (#3796)
c751a88 
* feat(ica): added EncodingJson to supported encodings

* imp(ica): changed the type of cdc to Codec in ica/host

* imp(ica): changed the type of cdc to Codec in ica/controller

* imp(ica.test): added a test cases for EncodingJSON

* imp(ica): created invalid encoding err

* feat(ica)!: first prototype of json supporting DeserializeCosmosTx

* docs(ica): updated godoc of DeserializeCosmosTx

* docs(ica): added comments to DeserializeCosmosTx

* fix(ica.test): fixed tests for DeserializeCosmosTx

* fix(ica): fixed 'OnRecvPacket' in relay.go

* fix(ica): fixed unhandled error

* style(ica): made DeserializeCosmosTx more compact

* fix(ica/host.cli.test): fixed a cli test

* style(ica): ran gofumpt

* style(ica): changed err message

* feat(ica): first prototype of SerializeCosmosTx is implemented

* fix(ica): fixed codec tests

* fix(ica/host.test): fix test

* fix(ica/host.test): fix test

* fix(ica/host.cli): cli always uses protobuf

* nil(ica/host.test): removed unneeded comment

* fix(ica/controller.test): fix test

* fix(ica/controller.test): fix test

* fix(ica/controller.test): fix test

* fix(fee.test): fix test

* nit: temporary save commit

* fix(ica): fixed json serde tests not passing

* fix(ica): fix panic if message does not implement sdk.Msg

* imp(ica): improved json serde functions

* style(ica): pleased the linter

* style(ica): ran gofumpt

* fix(e2e): fix compilation errors by adding icatypes.EncodingProtobuf arg to serde functions

* feat(ica.test): added important wip test for deserializing directly from cosmwasm

* imp(ica.test): added a new test case to cw codec unit test

* imp(ica): added another test case

* imp(ica.test): added another test case

* imp(ica.test): added another test case

* style(ica.test): improved test style

* style(ica.test): improved test style

* style(ica.test): ran gofumpt

* imp(ica.test): added json encoding version string for testing

* imp(ica.test): added new 'NewJSONICAPath' function

* imp(ica.test): added encoding field to ica test setup functions

* fix(ica.test): fixed test setups using the new encoding field

* feat(ica.test): added json test case

* style(ica.test): ran gofumpt

* feat(ica.test): got two cases of cosmwasm tests working in relay

* style(ica.test): ran gofumpt

* feat(ica): started progress on recursive handling of Anys

* imp(ica.test): added a new test case for ica json encoding, this fails

* feat(ica): achieved total json serialization (excluding any lists)

* refactor(ica): made function shorter and removed duplicated code

* style(ica): ran gofumpt

* imp(ica): added more err handling code

* refactor(ica): made deserialize code shorter

* style(ica): made linter a bit more happy

* fix(ica.test): fixed one codec test case

* feat(ica): added []Any handling code

* fix(ica): added more safety

* nit: deleted testing codec.go

* feat(ica): all works

* style(ica): ran gofumpt

* style(ica): made linter happy

* refactor(ica): reduced code duplication

* nit(ica): uncommented some test cases

* imp(ica.test): added more test cases

* feat(ica.test): finished test cases

* style(ica.test): reorganized test cases

* refactor(ica.test): combined the two test cases into one

* style(ica.test): ran gofumpt

* style(ica.test): renamed wallet address

* fix(ica.test): fixed test case names

* imp(ica.test): added more test cases

* style(ica.test): ran gofumpt

* test(ica): added more codec test cases

* style(ica.test): ran gofumpt

* feat(ica): removed JSONAny and JSONCosmosTx types

* feat(ica): implemented json encoding using module codec

* fix(ica.test): tests now match the new codec implementation

* fix(ica.test): fixed the tests to the new implementation

* style(ica.test): reorgenized the order of tests so that git diff makes sense

* imp(ica/controller): controller codec need not be codec.Codec

* imp(ica): replaced BinaryCodec with Codec

* test(ica): fixed codec test

* docs(ica.test): codec comment updated

* docs(ica.test): updated comments

* style(ica.test): removed 'from cosmwasm' from test case name as it is aparent from test name

* style(ica.test): ran gofumpt

* fix: fix merge error

* deps(ica): replaced sdk.NewInt with sdkmath.NewInt

* style(ica): ran 'gofumpt'

* imp(ica): removed redundant cosmwasm tests

* revert: "imp(ica): removed redundant cosmwasm tests"

This reverts commit 5123fba.

* imp(ica.test): made codec_test  human readable

* imp(ica.test): made relay_test human readable

* style(ica.test): ran 'golanci-lint run --fix'

* imp(ica/host): created 'GetAppMetadata' function

* refactor(ica/host): used GetAppMetadata function

* imp(ica.test): removed unneeded encoding argument

* imp(ica): removed ErrUnsupportedEncoding

* imp(ica.test): used suite chainB height instead of clienttypes.NewHeight(1, 100)

* imp(ica.test): add nil check for unsupported encoding

* imp(ica.test): added a empty/nil checks

* style(ica.test): renamed version variable to TestVersionWithJSONEncoding

* imp(ica): wrapped some errors

* style(ica): ran 'golanci-lint run --fix'

* style(ica)!: renamed EncodingJSON to EncodingProto3JSON

* docs(ica): improved godocs

* imp(ica): passing codec instead of binary codec

* style(ica): improved error messages and godocs

* docs(ica.test): improved godocs for tests

* imp(ica.test): improved unsupported encoding test case slightly

* style(ica.test): test style improvements

* imp(ica.test): added expError to some codec tests

* imp(ica.test): added more error type checks to codec tests

* style(ica.test): ran 'golangci-lint run --fix'

* imp(ica/host.test): added 'TestMetadataNotFound'

* imp(ica/host.test): reduce test size

* docs(ica/host.test): updated godocs for test

* docs(ica/host): improved godoc

* imp(ica/host): made GetAppMetadata private

---------

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit e5b057d)

# Conflicts:
#	modules/apps/27-interchain-accounts/controller/keeper/keeper.go
#	modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go
#	modules/apps/27-interchain-accounts/host/client/cli/tx_test.go
#	modules/apps/27-interchain-accounts/host/ibc_module_test.go
#	modules/apps/27-interchain-accounts/host/keeper/keeper.go
#	modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
#	modules/apps/27-interchain-accounts/host/keeper/relay.go
#	modules/apps/27-interchain-accounts/host/keeper/relay_test.go
#	modules/apps/27-interchain-accounts/types/codec.go
#	modules/apps/27-interchain-accounts/types/codec_test.go
@mergify mergify bot added the conflicts label Jul 4, 2023
colin-axner
colin-axner approved these changes Jul 5, 2023
View reviewed changes
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.

LGTM!

modules/apps/27-interchain-accounts/types/codec.go
func SerializeCosmosTx(cdc codec.BinaryCodec, msgs []proto.Message) (bz []byte, err error) {
// only ProtoCodec is supported
func SerializeCosmosTx(cdc codec.BinaryCodec, msgs []proto.Message) ([]byte, error) {
return SerializeCosmosTxWithEncoding(cdc, msgs, EncodingProtobuf)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is perfect 👍

modules/apps/27-interchain-accounts/host/keeper/keeper.go Outdated Show resolved Hide resolved
srdtrk and others added 2 commits July 5, 2023 17:54
@srdtrk @colin-axner
style(ica/host): Update modules/apps/27-interchain-accounts/host/keep…
2a3a515 
…er/keeper.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
@srdtrk
style(ica): ran gofumpt
0cf4e85
damiannolan
damiannolan approved these changes Jul 6, 2023
View reviewed changes
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.

LGTM!

@srdtrk srdtrk merged commit 6257a7f into release/v7.3.x Jul 6, 2023
@srdtrk srdtrk deleted the mergify/bp/release/v7.3.x/pr-3796 branch July 6, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-axner colin-axner colin-axner approved these changes

@damiannolan damiannolan damiannolan approved these changes

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal AdityaSripal is a code owner

Assignees

@srdtrk srdtrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@damiannolan @colin-axner @crodriguezvega @srdtrk