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

Integrate coin-type configuration #338

Merged
merged 5 commits into from
Nov 8, 2022
Merged

Integrate coin-type configuration #338

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c8232c
integrate coin-type configuration
boojamya Nov 3, 2022
5cf88df
account for cointype 0
boojamya Nov 4, 2022
e40e5b0
fix types
boojamya Nov 4, 2022
f530479
add comment
boojamya Nov 4, 2022
4f47884
handle feedback
boojamya Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions chain/cosmos/chain_node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ func (tn *ChainNode) CreateKey(ctx context.Context, name string) error {

_, _, err := tn.ExecBin(ctx,
"keys", "add", name,
"--coin-type", fmt.Sprint(tn.Chain.Config().CoinType),
"--keyring-backend", keyring.BackendTest,
)
return err
Expand All @@ -506,7 +507,7 @@ func (tn *ChainNode) RecoverKey(ctx context.Context, keyName, mnemonic string) e
command := []string{
"sh",
"-c",
fmt.Sprintf(`echo %q | %s keys add %s --recover --keyring-backend %s --home %s --output json`, mnemonic, tn.Chain.Config().Bin, keyName, keyring.BackendTest, tn.HomeDir()),
fmt.Sprintf(`echo %q | %s keys add %s --recover --keyring-backend %s --coin-type %s --home %s --output json`, mnemonic, tn.Chain.Config().Bin, keyName, keyring.BackendTest, tn.Chain.Config().CoinType, tn.HomeDir()),
}

tn.lock.Lock()
Expand Down Expand Up @@ -688,7 +689,7 @@ func (tn *ChainNode) ExecuteContract(ctx context.Context, keyName string, contra
return err
}

// QueryContract performs a smart query, taking in a query struct and returning a error with the response struct populated.
// QueryContract performs a smart query, taking in a query struct and returning a error with the response struct populated.
func (tn *ChainNode) QueryContract(ctx context.Context, contractAddress string, queryMsg any, response any) error {
query, err := json.Marshal(queryMsg)
if err != nil {
Expand Down
6 changes: 6 additions & 0 deletions chainspec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ func (s *ChainSpec) Config(log *zap.Logger) (*ibc.ChainConfig, error) {
// Apply any overrides from this ChainSpec.
cfg = cfg.MergeChainSpecConfig(s.ChainConfig)

coinType, err := cfg.VerifyCoinType()
if err != nil {
return nil, err
}
cfg.CoinType = coinType

// Apply remaining top-level overrides.
return s.applyConfigOverrides(cfg)
}
Expand Down
4 changes: 2 additions & 2 deletions ibc/relayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ import (
// but the report will be missing details.
type Relayer interface {
// restore a mnemonic to be used as a relayer wallet for a chain
RestoreKey(ctx context.Context, rep RelayerExecReporter, chainID, keyName, mnemonic string) error
RestoreKey(ctx context.Context, rep RelayerExecReporter, chainID, keyName, coinType, mnemonic string) error

// generate a new key
AddKey(ctx context.Context, rep RelayerExecReporter, chainID, keyName string) (Wallet, error)
AddKey(ctx context.Context, rep RelayerExecReporter, chainID, keyName, coinType string) (Wallet, error)

// GetWallet returns a Wallet for that relayer on the given chain and a boolean indicating if it was found.
GetWallet(chainID string) (Wallet, bool)
Expand Down
31 changes: 31 additions & 0 deletions ibc/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package ibc

import (
"reflect"
"strconv"

simappparams "github.com/cosmos/cosmos-sdk/simapp/params"
"github.com/cosmos/cosmos-sdk/types"
ibcexported "github.com/cosmos/ibc-go/v6/modules/core/03-connection/types"
Expand All @@ -22,6 +25,8 @@ type ChainConfig struct {
Bech32Prefix string `yaml:"bech32-prefix"`
// Denomination of native currency, e.g. uatom.
Denom string `yaml:"denom"`
// Coin type
CoinType string `default:"118" yaml:"coin-type"`
// Minimum gas prices for sending transactions, in native currency denom.
GasPrices string `yaml:"gas-prices"`
// Adjustment multiplier for gas fees.
Expand All @@ -46,6 +51,27 @@ func (c ChainConfig) Clone() ChainConfig {
return x
}

func (c ChainConfig) VerifyCoinType() (string, error) {
// If coin-type is left blank in the ChainConfig,
// the Cosmos SDK default of 118 is used.
if c.CoinType == "" {
typ := reflect.TypeOf(c)
f, _ := typ.FieldByName("CoinType")
coinType := f.Tag.Get("default")
Comment on lines +58 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels odd in the sense that I would expect the struct's field to take on the default value if the empty string is passed in. Idk that I've personally used the default tag before so maybe this is the idiomatic way to go about this?

Thoughts? cc @DavidNix @agouin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought so too! After testing, if the value is empty, it takes on "0" which is Bitcoin's coin-type.
If there is a better way to go about this, I'm happy to learn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems very counterintuitive. I'll go ahead and approve, if there is a better way to do this we can always circle back up on it. Good job!

_, err := strconv.ParseUint(coinType, 10, 32)
if err != nil {
return "", err
}
return coinType, nil
} else {
_, err := strconv.ParseUint(c.CoinType, 10, 32)
if err != nil {
return "", err
}
return c.CoinType, nil
}
}

func (c ChainConfig) MergeChainSpecConfig(other ChainConfig) ChainConfig {
// Make several in-place modifications to c,
// which is a value, not a reference,
Expand Down Expand Up @@ -77,6 +103,10 @@ func (c ChainConfig) MergeChainSpecConfig(other ChainConfig) ChainConfig {
c.Denom = other.Denom
}

if other.CoinType != "" {
c.CoinType = other.CoinType
}

if other.GasPrices != "" {
c.GasPrices = other.GasPrices
}
Expand Down Expand Up @@ -178,6 +208,7 @@ type Wallet struct {
Mnemonic string `json:"mnemonic"`
Address string `json:"address"`
KeyName string
CoinType uint32
}

func (w *Wallet) GetKeyName() string {
Expand Down
14 changes: 9 additions & 5 deletions interchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ibctest
import (
"context"
"fmt"
"strconv"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
Expand Down Expand Up @@ -423,6 +424,7 @@ func (ic *Interchain) configureRelayerKeys(ctx context.Context, rep *testreporte
if err := r.RestoreKey(ctx,
rep,
c.Config().ChainID, chainName,
c.Config().CoinType,
ic.relayerWallets[relayerChain{R: r, C: c}].Mnemonic,
); err != nil {
return fmt.Errorf("failed to restore key to relayer %s for chain %s: %w", ic.relayers[r], chainName, err)
Expand All @@ -442,9 +444,11 @@ type relayerChain struct {
// BuildWallet will generate a random key for the key name in the provided keyring.
// Returns the mnemonic and address in the bech32 format of the provided ChainConfig.
func BuildWallet(kr keyring.Keyring, keyName string, config ibc.ChainConfig) ibc.Wallet {
// NOTE: this is hardcoded to the cosmos coin type.
// In the future, we may need to get the coin type from the chain config.
const coinType = types.CoinType
coinTypeU64, err := strconv.ParseUint(config.CoinType, 10, 32)
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable is named coinTypeU64 but you are telling ParseUInt to use 32bits, and then on L451 there's a typecast to uint32. It looks like the coin type is a uint32 so I'd say this parse call is fine but the type cast could be eliminated.

Also the name could be shortened to just coinType for brevity

Copy link
Contributor Author

@boojamya boojamya Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ParseUInt outputs a typed uint64 integer whether you specify 32bits or 64bits.
That hd.CreateHdPath function takes a uint32. This is the reason for the type cast.

Regardless, for readability, I like what you're saying here and can do the type cast directly in the hd.CreateHdPath function (L456) with the renaming suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well TIL! Disregard my comment then 😛

if err != nil {
panic(fmt.Errorf("invalid coin type: %w", err))
}
coinType := uint32(coinTypeU64)

info, mnemonic, err := kr.NewMnemonic(
keyName,
Expand All @@ -463,9 +467,9 @@ func BuildWallet(kr keyring.Keyring, keyName string, config ibc.ChainConfig) ibc
}

return ibc.Wallet{
Address: types.MustBech32ifyAddressBytes(config.Bech32Prefix, addr.Bytes()),

Address: types.MustBech32ifyAddressBytes(config.Bech32Prefix, addr.Bytes()),
Mnemonic: mnemonic,
CoinType: coinType,
}
}

Expand Down
12 changes: 6 additions & 6 deletions relayer/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ func (r *DockerRelayer) generateConfigTar(relativeConfigPath string, content []b
return &buf, nil
}

func (r *DockerRelayer) AddKey(ctx context.Context, rep ibc.RelayerExecReporter, chainID, keyName string) (ibc.Wallet, error) {
cmd := r.c.AddKey(chainID, keyName, r.HomeDir())
func (r *DockerRelayer) AddKey(ctx context.Context, rep ibc.RelayerExecReporter, chainID, keyName, coinType string) (ibc.Wallet, error) {
cmd := r.c.AddKey(chainID, keyName, coinType, r.HomeDir())

// Adding a key should be near-instantaneous, so add a 1-minute timeout
// to detect if Docker has hung.
Expand Down Expand Up @@ -317,8 +317,8 @@ func (r *DockerRelayer) Exec(ctx context.Context, rep ibc.RelayerExecReporter, c
}
}

func (r *DockerRelayer) RestoreKey(ctx context.Context, rep ibc.RelayerExecReporter, chainID, keyName, mnemonic string) error {
cmd := r.c.RestoreKey(chainID, keyName, mnemonic, r.HomeDir())
func (r *DockerRelayer) RestoreKey(ctx context.Context, rep ibc.RelayerExecReporter, chainID, keyName, coinType, mnemonic string) error {
cmd := r.c.RestoreKey(chainID, keyName, coinType, mnemonic, r.HomeDir())

// Restoring a key should be near-instantaneous, so add a 1-minute timeout
// to detect if Docker has hung.
Expand Down Expand Up @@ -644,7 +644,7 @@ type RelayerCommander interface {
// The remaining methods produce the command to run inside the container.

AddChainConfiguration(containerFilePath, homeDir string) []string
AddKey(chainID, keyName, homeDir string) []string
AddKey(chainID, keyName, coinType, homeDir string) []string
CreateChannel(pathName string, opts ibc.CreateChannelOptions, homeDir string) []string
CreateClients(pathName string, opts ibc.CreateClientOptions, homeDir string) []string
CreateConnections(pathName, homeDir string) []string
Expand All @@ -655,7 +655,7 @@ type RelayerCommander interface {
GetChannels(chainID, homeDir string) []string
GetConnections(chainID, homeDir string) []string
LinkPath(pathName, homeDir string, channelOpts ibc.CreateChannelOptions, clientOpts ibc.CreateClientOptions) []string
RestoreKey(chainID, keyName, mnemonic, homeDir string) []string
RestoreKey(chainID, keyName, coinType, mnemonic, homeDir string) []string
StartRelayer(homeDir string, pathNames ...string) []string
UpdateClients(pathName, homeDir string) []string
}
9 changes: 5 additions & 4 deletions relayer/rly/cosmos_relayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package rly
import (
"context"
"encoding/json"
"fmt"
"strings"

"github.com/cosmos/cosmos-sdk/crypto/keyring"
Expand Down Expand Up @@ -114,10 +115,10 @@ func (commander) AddChainConfiguration(containerFilePath, homeDir string) []stri
}
}

func (commander) AddKey(chainID, keyName, homeDir string) []string {
func (commander) AddKey(chainID, keyName, coinType, homeDir string) []string {
return []string{
"rly", "keys", "add", chainID, keyName,
"--home", homeDir,
"--coin-type", fmt.Sprint(coinType), "--home", homeDir,
boojamya marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -212,10 +213,10 @@ func (commander) LinkPath(pathName, homeDir string, channelOpts ibc.CreateChanne
}
}

func (commander) RestoreKey(chainID, keyName, mnemonic, homeDir string) []string {
func (commander) RestoreKey(chainID, keyName, coinType, mnemonic, homeDir string) []string {
return []string{
"rly", "keys", "restore", chainID, keyName, mnemonic,
"--home", homeDir,
"--coin-type", fmt.Sprint(coinType), "--home", homeDir,
}
}

Expand Down