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.

• [x] 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.
• [x] 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

Description

This PR updates the linters so that we no longer use deprecated linters, enforces linting in CI, and fixes linter errors.

closes: #1417

Please note that this changes a number of important names, but likely most important:

every time we named something with Id, that has been changed to ID per revive.

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.

• [x] Targeted PR against correct branch (see CONTRIBUTING.md)
• [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• [x] Code follows the module structure standards.
• [ ] Wrote unit and integration tests
• [ ] Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• [ ] Added relevant godoc comments.
• [x] Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• [x] Re-reviewed Files changed in the Github PR explorer
• [x] Review Codecov Report in the comment section below once CI passes

Summary

Support a larger amount field on FungibleTokenPacketData instead of uint64. This would make it's support in line with what MsgTransfer.Amount supports.

The SDK currently panics when trying to convert a large MsgTransfer.Amount to the underlying FungibleTokenPacketData.Amount.

Problem Definition

Transferring tokens with 18 decimal places, like many Ethereum tokens for instance, are limited to insignificant amounts (< $1).

Transferring these tokens by splitting the transfer into multiple smaller transfers would require several hundred or thousands messages.

The fees increase significantly to an uneconomical state when needing to split a transfer to this degree.

Keprl also does not handle this well in the UI, as this many messages shouldn't really be a use case.

Some relayers also don't handle these large transactions well.

Proposal

Introduce a larger amount field (i.e string) on FungibleTokenPacketData proto message, to replace the current amount field.

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

Summary

02-client should not set client states and consensus states as generalizing IBC client implementations is difficult. Better to give IBC clients more control and flexibility. This will already be useful for solo machines which don't need to store consensus states for each update height

This design is already specified in ICS, our implementation didn't follow those changes. See comment

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

The scopedKeeper in all Keepers in ibc-go has at the moment a concrete type (i.e. capabilitykeeper.ScopedKeeper), but there is no good reason why we should not use an interface instead. Other keepers in the Keeper types are already using interfaces.

Proposal

1. Add ScopedKeeper interface.

For 05-port create an expected_keepers.go file in package types and add the following interface:

type ScopedKeeper interface {
  NewCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, error)
  GetCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, bool)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

For 04-channel add the following interface to expected_keepers.go:

type ScopedKeeper interface {
  NewCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, error)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

For transfer add the following interface to expected_keepers.go:

type ScopedKeeper interface {
  GetCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, bool)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

For 27-interchain-accounts add the following interface to expected_keepers.go:

type ScopedKeeper interface {
  GetCapability(ctx sdk.Context, name string) (*capabilitytypes.Capability, bool)
  AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool
  LookupModules(ctx sdk.Context, name string) ([]string, *capabilitytypes.Capability, error)
}

where capabilitytypes is alias for github.com/cosmos/cosmos-sdk/x/capability/types.

2. Change the type for the scopedKeeper field in the Keeper type of:

• 05-port - PR
• 04-channel - PR
• transfer - PR
• 27-interchain-accounts controller and 27-interchain-accounts host - PR

to the corresponding interface type created on step 1.

Notes

• Consider opening different PRs for the changes affecting each part of the codebase.
• This is an API breaking change.

Description

I'd like to get this into a tagged release form after review

This is a notional-umee collaboration.

On today's cosmos SDK call there was some lamentation of the lack of 46 support, and Robert pointed out something pretty important to me: the v4/main I am building against in https://github.com/cosmos/ibc-go/pull/1653 isn't yet stable and that could lead to yet more unplesantry.

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

Description

Allow packets to include an optional metadata field.

This PR introduces the data changes. A new PR will add the integration and tests.

closes: #2411

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

Summary of Bug

After I do an ibv-upgrade with changed chain_id to rev 2 and starting height from block 1, I get the following error:

{
    "height": "27",
    "txhash": "F3C47BDE2CFFF22E3FF9CC782F74E7CEABE4B570FDE7E36BAC88063088B032A7",
    "codespace": "channel",
    "code": 9,
    "data": "",
    "raw_log": "failed to execute message; message index: 0: module does not own channel capability: channel capability not found",
    "logs": [],
    "info": "",
    "gas_wanted": "200000",
    "gas_used": "43092",
    "tx": null,
    "timestamp": ""
}

I found a simmilar issue here.

In my case though, creating a new channel and transferring over it works fine.

Version

Cosmos sdk v0.44.3, Ibc-go v1.1, Hermes relayer v0.9

Steps to Reproduce

1. Get 2 chains and a relayer up.
2. Create clients and channel between them.
3. Make an ibc-upgrade proposal, with client state that is exported from chain b and has the following fields edited:

• chain_id to rev 2
• trusting_period set to 0
• frozen_height revision_height to 1 block before the upgrade block
• latest_height revision_number to 2 and revision_height to 1

4. Vote and pass the upgrade
5. When chain a halts, upgrade client from the Hermes relayer
6. Replace chain a binary with one that has a basic upgrade handler for the upgrade:

	app.UpgradeKeeper.SetUpgradeHandler("val", func(ctx sdk.Context, plan upgradetypes.Plan, a module.VersionMap) (module.VersionMap, error) {
		return a, nil
	})

7. Export the genesis, change the chain_id and starting-height and replace the old one with it.
8. unsafe-reset-all and start chain a
9. Try to send token transfer from chain a to chain b over the existing channel.

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

Summary

The IBC cli is not being used or tested. It should be refactored to support more dynamic usage.

Problem Definition

While working through 03-connection I noticed the cmd for the OpenTry handshake passed in proofInit to the proofConsensus field. Any attempt at using this command successfully should fail showing that no one has attempted to use this command (which is essential to doing anything IBC).

The iqlusioninc relayer, one of the more widely used relayers, does not rely on these commands and instead has its own custom cli commands. There should be at least a few usable commands it could import instead of implementing all of its own. This should be true for any relayer.

Proposal

Refactor and test the IBC cli/rest commands with usability in mind. Currently proofs are passed in directly or by json file. There should be an option to pass in a connection endpoint to the counterparty chain to query and parse the proofs directly. Perhaps there should also be commands to query and store proofs (if there isn't already)

This should probably be punted until after the gRPC refactor across the sdk codebase.

A concrete list of things that need to be done to close this issue:

manually test the following cmds:

• [x] create client
• [x] update client
• [x] client misbehaviour
• [ ] upgrade client
• [x] transfer tx cmd
• [ ] transfer query demon trace
• [ ] transfer query denom traces
• [x] query client state
• [x] query client states
• [x] query consensus state
• [x] query consensus states
• [ ] query header
• [x] query node consensus state
• [x] query connection
• [x] query connections
• [x] query client connections
• [x] query channel
• [x] query channels
• [x] query connection channels
• [x] query channel client state
• [x] query packet commitment
• [x] query packet commitments
• [ ] query unreceived packets
• [ ] query unrelayed acks
• [x] query next sequence recv

cmds to add and test:

• [ ] relay packet
• [ ] relay acknowledgement

Also consider applying the original proposal of this issue, allowing an endpoint to be passed/queried for counterparty proofs. Maybe this can be opened in a separate issue and implemented if users request the same feature

Summary of Bug

The following issue was reported on ibc-rs https://github.com/informalsystems/ibc-rs/issues/2433. It refers to failures for ConnOpenAck reported by ibc-go. The failure can also appear in ConnOpenTry. They are more obvious when the chain block times are significantly different, I am able to repro easily with 1s and 10s for chains A and B respectively. The failures seen are caused by try check: https://github.com/cosmos/ibc-go/blob/aaccbd193f19f97f8068278141044bf324e6425c/modules/core/03-connection/keeper/handshake.go#L79
and the ack check: https://github.com/cosmos/ibc-go/blob/aaccbd193f19f97f8068278141044bf324e6425c/modules/core/03-connection/keeper/handshake.go#L169 ) in the on-chain handlers.

They show up while hermes tries to simulate the transaction. It is not clear to me why the check is for >= and not >. It is possible to update the client on chain A with a header from B, get the consensus proof and submit a ConnOpenTry Tx for simulation to chain B before B advances to a new height. Would the > check be to loose for the deliverTx?

Expected Behaviour

Version

Steps to Reproduce

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

Description

Returns sequence from sendTransfer, and returns it with the MsgTransferResponse. This is not an API breaking change.

Retrieving the sequence at the time of creating the transfer is necessary in the packet forward middleware for correlation with multihop packet flows.

https://github.com/strangelove-ventures/packet-forward-middleware/pull/33 https://github.com/strangelove-ventures/ibctest/pull/306

Closes #1969

• [x] Targeted PR against correct branch (see CONTRIBUTING.md)
• [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• [x] Code follows the module structure standards.
• [x] Wrote unit and integration tests Existing test coverage exercises this new method due to the re-routing of SendTransfer through SendPacketTransfer
• [x] Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• [x] Added relevant godoc comments.
• [x] Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• [x] Re-reviewed Files changed in the Github PR explorer
• [x] Review Codecov Report in the comment section below once CI passes

Description

Adds limitations to the length of ibc receive addresses.

A 2kb size was chosen to preserve the flexible use of the receive address field, where it's just a string and can be interpreted by various modules to trigger other functionality.

TODO:

• use error conventionally
• write a test

Commit Message / Changelog Entry

fix: [#2983](https://github.com/cosmos/ibc-go/pull/2983) Limits the receive address length to 2kb

see the guidelines for commit messages. (view raw markdown for examples)

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.
• [ ] Provide a commit message to be used for the changelog entry in the PR description for review.
• [ ] Re-reviewed Files changed in the Github PR explorer.
• [ ] Review Codecov Report in the comment section below once CI passes.

Description

closes: #1667

Move the ibc module name into the exported package

Commit Message / Changelog Entry

chore: Moved the ibc module name into the exported package

see the guidelines for commit messages. (view raw markdown for examples)

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.
• [ ] Provide a commit message to be used for the changelog entry in the PR description for review.
• [ ] Re-reviewed Files changed in the Github PR explorer.
• [ ] Review Codecov Report in the comment section below once CI passes.

Description

closes: #1856

Commit Message / Changelog Entry

docs: light client dev guide `CheckSubstituteAndUpdateState`

see the guidelines for commit messages. (view raw markdown for examples)

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.
• [ ] Provide a commit message to be used for the changelog entry in the PR description for review.
• [ ] Re-reviewed Files changed in the Github PR explorer.
• [ ] Review Codecov Report in the comment section below once CI passes.

Summary of Bug

add some code snippets / psuedocode for VerifyMembership and VerifyNonMembership in the light client guide (proofs section), explain ics24, seeing as key paths are standardised for ibc and implementation of ics24 is key to IBC interoperability

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

Summary of Bug

The ICA CLI commands are always added for both host and controller (see query and tx) even if one of them is not enabled on the given chain.

The occurence of e.g. the controller subcommands are confusing when expecting only the host submodule to be there. Executing a non-supported command returns

 $ simd query interchain-accounts controller params
Error: rpc error: code = Unknown desc = unknown query path: unknown request
Usage:
  simd query interchain-accounts controller params [flags]
...

This is to expected given how cobra works, but to someone who does not have the full background on this, it is simply confusing because the shown usage is exactly what was put into the command line.

Expected Behaviour

Only show the commands for the enabled submodules or return a meaningful error message (e.g. interchain-accounts controller disabled or similar).

Version

Ever since ICA CLI commands were introduced.

Steps to Reproduce

1. Add ICA module to Cosmos based chain
2. Enable e.g. ICA host but disable ICA controller
3. simd query interchain-accounts controller params

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

we need to keep in sync with using full-scoped interface names in protos (already merged in cosmos-sdk)

• reference PR
• issue describing problem
• new updated docs with required changes