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

Feature/ add substrate queries #942

Merged
merged 6 commits into from
Oct 24, 2022

Conversation

TxCorpi0x
Copy link

@TxCorpi0x TxCorpi0x commented Aug 23, 2022

Description

Query methods are available in the query.go in the substrate package and it's methods are being used by the substrate package and the rest of the packages of the relayer code-base.

Added Methods to query.go

  • QueryBalance
  • QueryBalanceWithAddress
  • QueryClientStateResponse
  • QueryClientState
  • QueryClientConsensusState
  • QueryUpgradeProof
  • QueryUpgradedClient
  • QueryUpgradedConsState
  • QueryConsensusState
  • QueryConnection
  • QueryConnections
  • QueryConnectionsUsingClient
  • GenerateConnHandshakeProof
  • QueryChannel
  • QueryChannelClient
  • QueryConnectionChannels
  • QueryChannels
  • QueryPacketCommitments
  • QueryPacketAcknowledgements
  • QueryUnreceivedPackets
  • QuerySendPacket
  • QueryRecvPacket
  • QueryUnreceivedAcknowledgements
  • QueryNextSeqRecv
  • QueryPacketCommitment
  • QueryPacketAcknowledgement
  • QueryPacketReceipt
  • QueryLatestHeight
  • QueryHeaderAtHeight
  • QueryDenomTrace
  • QueryDenomTraces
  • QueryConsensusStateABCI

@blasrodri
Copy link

blasrodri commented Aug 31, 2022

@jtieri
I'm getting after solving conflicts

go: finding module for package github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx
github.com/cosmos/relayer/v2/relayer/chains/cosmos imports
	github.com/tendermint/tendermint/rpc/core/types: module github.com/tendermint/tendermint@latest found (v0.35.9), but does not contain package github.com/tendermint/tendermint/rpc/core/types
github.com/cosmos/relayer/v2/relayer/chains/substrate imports
	github.com/cosmos/ibc-go/v5/modules/light-clients/11-beefy/types: module github.com/cosmos/ibc-go/v5@latest found (v5.0.0-rc1), but does not contain package github.com/cosmos/ibc-go/v5/modules/light-clients/11-beefy/types
github.com/cosmos/relayer/v2/relayer/chains/cosmos imports
(truncated)

any idea on how to fix it?

TxCorpi0x and others added 4 commits October 11, 2022 23:39
# Conflicts:
#	go.mod
#	go.sum
#	ibctest/go.mod
#	ibctest/go.sum
#	relayer/chains/substrate/construct.go
#	relayer/chains/substrate/errors.go
#	relayer/chains/substrate/event_parser.go
#	relayer/chains/substrate/provider.go
Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

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

Just had one question and a couple nit picks but this looks good otherwise. I'll approve and leave it up to you if you wanna address the few comments I left now or later.


// QueryUnbondingPeriod returns the unbonding period of the chain
func (sp *SubstrateProvider) QueryUnbondingPeriod(ctx context.Context) (time.Duration, error) {
return 0, nil
Copy link
Member

Choose a reason for hiding this comment

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

After some light Googling I see that Polkadot has an unbonding period of 28 days, my naive understanding is that all stake in Polkadot is locked up on the relay chain so does that mean the 28 day unbonding period applies in all cases no matter what Parachain may be actually connecting to IBC?

If so could this be hardcoded to 28 days?

Choose a reason for hiding this comment

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

yes, the unbonding period would apply to all parachains secured by the relaychain. However, the duration of the unbonding period is dependent on the network. For instance Kusama has a 7 day unbonding period. So we'll probably need to use a switch statement for that. I'll add it in another PR.

// QueryBalanceWithAddress returns the amount of coins in the relayer account with address as input
// TODO add pagination support
func (sp *SubstrateProvider) QueryBalanceWithAddress(ctx context.Context, address string) (sdk.Coins, error) {
// TODO: addr might need to be passed as byte not string
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this TODO may no longer be needed

if keyName == "" {
addr, err = sp.Address()
} else {
sp.Config.Key = keyName
Copy link
Member

Choose a reason for hiding this comment

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

This would change the SubstrateProvider's configured key in memory but not on disk so later operations could be referencing two different keys, the one on disk in the config file or the one now referenced in memory. As a consumer of this API it would be an unexpected side effect for a method named QueryBalance to modify state instead of just querying some data. In the cosmos implementation we just throw an error if the keyName argument is invalid for any reason whether it's an empty string or there is no key that matches the name.

Choose a reason for hiding this comment

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

Yeah, that makes sense. I'll resolve it in another PR.

@jtieri jtieri merged commit c0bdd01 into cosmos:feat-substrate Oct 24, 2022
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.

4 participants