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

Add test for Ledger support #4783

Open
2 of 3 tasks
chatton opened this issue Sep 28, 2023 · 3 comments
Open
2 of 3 tasks

Add test for Ledger support #4783

chatton opened this issue Sep 28, 2023 · 3 comments
Labels
e2e testing Testing package and unit/integration tests type: productivity Increase dev productivity and throughput by improving developer tooling, infrastructure, automation

Comments

@chatton
Copy link
Contributor

chatton commented Sep 28, 2023

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:

  • Optionally configure the devices/privelaged field here. This is where containers that run individual commands get their configuration.
  • modify create key to support the ledger field (or add a new function to create a ledger key specifically)
  • Updating the broadcaster type to not always require a keyring. I'm not 100% sure what the expected keyring behaviour is when using ledger so more investigation is needed here.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@chatton chatton added testing Testing package and unit/integration tests e2e labels Sep 28, 2023
@chatton
Copy link
Contributor Author

chatton commented Sep 28, 2023

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)
}

@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Oct 6, 2023
@damiannolan
Copy link
Member

damiannolan commented Jun 6, 2024

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
Copy link
Contributor Author

chatton commented Jun 6, 2024

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.

@crodriguezvega crodriguezvega added the type: productivity Increase dev productivity and throughput by improving developer tooling, infrastructure, automation label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e testing Testing package and unit/integration tests type: productivity Increase dev productivity and throughput by improving developer tooling, infrastructure, automation
Projects
Status: Backlog 🕐
Development

No branches or pull requests

3 participants