cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
551 stars 586 forks source link

Add test for Ledger support #4783

Open chatton opened 1 year ago

chatton commented 1 year ago

Summary

I propose we add a test (using interchain test) to test ledger support.

This would not be run on PRs, but would be something that can be manually triggered. I experimented briefly with the idea, and there are a few things that would be some changes needed to interchaintest to support this behaviour.

Firstly, there is an open issue that prevents this from working at all on a Mac, the test would have to be run from a linux host machine (or at least using a linux machine with the DOCKER_HOST environment variable )

I was able to add a ledger key successfully with the following command

docker run --device /dev/bus/usb/  ghcr.io/cosmos/ibc-go-simd:main keys add ledgerKey --ledger --keyring-backend test

Note: the following also works

docker run --privileged  ghcr.io/cosmos/ibc-go-simd:main keys add ledgerKey --ledger --keyring-backend test

The general idea for the test would be to configurable address which is specified in a config file/env, fund that address in setup. Perform a regular transfer test and have the tx signed manually using the ledger as the test runs.

All of this should be possible with just a few modifications to interchain test.

Changes in interchain test that would be required for this to be possible:


For Admin Use

chatton commented 1 year ago

Adding some sample code I was experimenting with as a reference

func TestLedgerTestSuite(t *testing.T) {
    testifysuite.Run(t, new(LedgerTestSuite))
}

type LedgerTestSuite struct {
    testsuite.E2ETestSuite
}

// CreateLedgerKey creates a key in the keyring backend test for the given node
func CreateLedgerKey(ctx context.Context, name string, chain *cosmos.CosmosChain) error {
    _, _, err := chain.Nodes()[0].ExecBin(ctx,
        "keys", "add", name,
        "--coin-type", chain.Config().CoinType,
        "--keyring-backend", keyring.BackendTest,
        "--ledger",
        "--account", "1",
    )
    return err
}

func GetAndFundLedgerWallet(
    ctx context.Context,
    amount int64,
    chain ibc.Chain,
) (ibc.Wallet, error) {
    chainCfg := chain.Config()
    keyName := "ledgerKey"

    if err := CreateLedgerKey(ctx, keyName, chain.(*cosmos.CosmosChain)); err != nil {
        return nil, fmt.Errorf("failed to create key %q on chain %s: %w", keyName, chainCfg.Name, err)
    }

    addrBytes, err := chain.GetAddress(ctx, keyName)
    if err != nil {
        return nil, fmt.Errorf("failed to get account address for key %q on chain %s: %w", keyName, chain.Config().Name, err)
    }

    wallet := cosmos.NewWallet(keyName, addrBytes, "", chain.Config())
    err = chain.SendFunds(ctx, interchaintest.FaucetAccountKeyName, ibc.WalletAmount{
        Address: wallet.FormattedAddress(),
        Amount:  sdkmath.NewInt(amount),
        Denom:   chainCfg.Denom,
    })

    if err != nil {
        return nil, fmt.Errorf("failed to get funds from faucet: %w", err)
    }

    return wallet, nil
}

func (s *LedgerTestSuite) TestLedger() {
    t := s.T()
    ctx := context.TODO()

    relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, s.TransferChannelOptions())
    chainA, chainB := s.GetChains()

    chainADenom := chainA.Config().Denom

    ledgerWallet, err := GetAndFundLedgerWallet(ctx, testvalues.StartingTokenAmount, chainA)
    s.Require().NoError(err)
    ledgerAddress := ledgerWallet.FormattedAddress()
    s.T().Logf("ledgerAddress: %s", ledgerAddress)
}
damiannolan commented 5 months ago

Awesome that you already have some sample code and an issue for this @chatton! Great work! Did you ever get to signing a tx within an e2e test? I assume if you run this locally with a ledger device connected and try to broadcast tx that you would be prompted on the ledger to sign when the time comes.

Would love to see this issue get some more attention!

edit: I see in the main issue body that maybe some changes are required to interchaintest. Maybe we could open some issues or PRs over there to accelerate it

chatton commented 5 months ago

Great work! Did you ever get to signing a tx within an e2e test? I assume if you run this locally with a ledger device connected and try to broadcast tx that you would be prompted on the ledger to sign when the time comes.

Yes I think I got it prompting for the sign on the ledger that was connected, would love to see this issue get more attention, think it would save a lot of time to have some e2es we can manually run to verify ledger support.