feat: introduce cometbft client wrapper to eliminate forked repo #1384
Conversation
| @@ -25,7 +25,7 @@ func (cc *CosmosProvider) GetAccount(clientCtx client.Context, addr sdk.AccAddre | |||
| // GetAccountWithHeight queries for an account given an address. Returns the | |||
| // height of the query with the account. An error is returned if the query | |||
| // or decoding fails. | |||
| func (cc *CosmosProvider) GetAccountWithHeight(clientCtx client.Context, addr sdk.AccAddress) (client.Account, int64, error) { | |||
| func (cc *CosmosProvider) GetAccountWithHeight(_ client.Context, addr sdk.AccAddress) (client.Account, int64, error) { | |||
There was a problem hiding this comment.
It seems like we should use the context here in queryClient.Account on line 36 instead of creating anew background context?
There was a problem hiding this comment.
i actually thought that too but realized the Account method expects a normal context whereas the context being passed into GetAccountWithHeight is a type from the cosmos sdk.
relayer/chains/cosmos/tx.go
Outdated
| @@ -1305,9 +1358,22 @@ func (cc *CosmosProvider) QueryICQWithProof(ctx context.Context, path string, re | |||
| if err != nil { | |||
| return provider.ICQProof{}, fmt.Errorf("failed to execute interchain query: %w", err) | |||
| } | |||
|
|
|||
| proofOps := &crypto.ProofOps{} | |||
There was a problem hiding this comment.
Is this needed anymore? If so, as a method within the relayer client package would be good. This can also start as a nil slice to be consistent var proofOps crypto.ProofOps
There was a problem hiding this comment.
Nope, we no longer need any of the explicit proof ops conversions in the provider implementations since the client wrapper does this now.
| @@ -600,10 +606,22 @@ func (cc *CosmosProvider) QueryConsensusState(ctx context.Context, height int64) | |||
| return &tmclient.ConsensusState{}, 0, err | |||
| } | |||
|
|
|||
| // TODO: this may not work | |||
There was a problem hiding this comment.
I missed this, but this is no longer needed anymore either I believe
This PR introduces a CometBFT client wrapper, see cometbft-client. This allows us to handle breaking changes in CometBFT without being limited by the Go module system or having to introduce a forked repo, like we have been using.
The imported
cometbft-clientlibrary wraps the Comet RPC client and mutates the returned data with one of the current main goals being to handle the breakage inResultBlockResults, see #1273. Our client will also internally handle the base64 decoding of events so that we can handle all events as strings regardless of what version of CometBFT a chain is using.On the relayer side we also introduce a wrapper around this new client. This allows us to satisfy the
CometRPCinterface by making calls into our client and adapting the returned types to the appropriate upstream CometBFT types.I updated the conformance tests to run against current versions of the Cosmos Hub and Osmosis and don't see the errors described in #1354 with these changes, but we may want to also test against a live network to be sure.
Closes #1354