Adding support for `RecoverKey` in the chain interface. #176

chatton · 2022-06-27T14:33:27Z

Description

This PR adds the functionality to recover a test user from a given mnemonic.

Checklist

I have made sure the upstream branch for this PR is correct
I have made sure this PR is ready to merge
I have made sure that I have assigned reviewers related to this project

Changes

Added RecoverKey to ChainNode and CosmosChain types.
Added RecoverKey to Chain interface.
~~Added Shell field to ChainConfig. This is to allow you to specify which shell commands which run using -c <script> will use, as not all images will have the same shells.~~
Added GetAndFundTestUserWithMnemonic which generates a user using a given mnemonic.
Changed GetAndFundTestUsers to delegate to GetAndFundTestUserWithMnemonic
Changed generateUserWallet to either Create or Recover based on the presence of a mnemonic.

Testing

Tests have been added which verify that users can be created using CreateKey and RecoverKey

closes #169

…helper method which generates a user using a provided mnemonic.

chatton · 2022-06-27T14:36:05Z

chain/penumbra/penumbra_chain.go

+func (c *PenumbraChain) RecoverKey(ctx context.Context, name, mnemonic string) error {
+	return fmt.Errorf("RecoverKey not implemented for PenumbraChain")
+}


I'm not sure exactly what would need to be done here.

I think this error is fine for now.

Yup, Penumbra is highly experimental right now.

mark-rushakoff

There are a few things to clean up, but I think we'll be able to get this merged soon.

mark-rushakoff · 2022-06-27T15:09:41Z

chain/penumbra/penumbra_chain.go

+func (c *PenumbraChain) RecoverKey(ctx context.Context, name, mnemonic string) error {
+	return fmt.Errorf("RecoverKey not implemented for PenumbraChain")
+}


I think this error is fine for now.

mark-rushakoff · 2022-06-27T15:24:58Z

test_user.go

+		Denom:   chainCfg.Denom,
+	})
+	require.NoError(t, err, "failed to get funds from faucet")
+	require.NoError(t, test.WaitForBlocks(ctx, 5, chain), "failed to wait for blocks")


Did 5 come from anywhere special? Would 4 or 3 work?

Some of these tests can take a long time to complete, so if there is a way to minimize time spent waiting, we should take it.

Maybe it would make more sense to not wait inside this function, and indicate in Godoc that the caller should wait for some blocks to complete before the funds will be accessible. There is one wait at the end of GetAndFundTestUsers that would be redundant with this call, if it stays.

This wait was just taken from the existing GetAndFundTestUsers function, however I think it's no harm removing it and letting the caller know as you suggested. I can remove from both.

mark-rushakoff · 2022-06-27T15:27:51Z

interchain_test.go

+		require.NotEmpty(t, users[0].KeyName)
+	})
+
+	_ = ic.Close()


Preferable to defer ic.Close() immediately after the initial assignment.

If the linter complains about a discarded error value, defer func() { _ = ic.Close() } is fine.

mark-rushakoff · 2022-06-27T15:29:46Z

ibc/types.go

+// GetShell returns the shell that should be used when executing scripts.
+func (c ChainConfig) GetShell() string {
+	if c.Shell == "" {
+		return "sh"
+	}
+	return c.Shell
+}
+


For now, I think we should assume all containers have sh available. It will likely be a long time before we run into something we can't do with plain sh.

I agree. The benefit is keeping ChainConfig simple.

mark-rushakoff · 2022-06-27T15:37:37Z

interchain_test.go

+	ic := ibctest.NewInterchain().
+		AddChain(gaia0).
+		AddChain(gaia1).
+		AddRelayer(r, "r").
+		AddLink(ibctest.InterchainLink{
+			Chain1:  gaia0,
+			Chain2:  gaia1,
+			Relayer: r,
+		})


Because this test is only exercising funding a user, I think this can be reduced to ic := ibctest.NewInterchain().AddChain(gaia0).

This should be testable even without an Interchain, but since that does the heavy lifting to start a chain, it's probably more convenient than calling several methods directly on a chain.

mark-rushakoff · 2022-06-27T15:37:57Z

interchain_test.go

+		NetworkID: network,
+	}))
+
+	t.Run("Test User creation with mnemonic", func(t *testing.T) {


Names of subtests should be shorter because they are displayed after the outer test's name. This line can be t.Run("with mnemonic", ...).

mark-rushakoff · 2022-06-27T15:40:23Z

interchain_test.go

+		require.NotEmpty(t, user.KeyName)
+	})
+
+	t.Run("Test user creation without mnemonic", func(t *testing.T) {


t.Run("without mnemonic", ...)

interchain_test.go

mark-rushakoff · 2022-06-27T15:47:17Z

chain/cosmos/chain_node.go

+// RecoverKey restores a key from a given mnemonic.
+func (tn *ChainNode) RecoverKey(ctx context.Context, keyName, mnemonic string) error {
+	command := []string{
+		tn.Chain.Config().GetShell(),


This can be hardcoded to "sh" after removing the GetShell method.

mark-rushakoff · 2022-06-27T15:49:53Z

chain/cosmos/chain_node.go

+	command := []string{
+		tn.Chain.Config().GetShell(),
+		"-c",
+		fmt.Sprintf(`echo "%s" | %s keys add %s --recover --keyring-backend %s --home %s --output json`, mnemonic, tn.Chain.Config().Bin, keyName, keyring.BackendTest, tn.NodeHome()),


echo %q | ... should be safer, as that will surround the string with quotes and add some escaping of special characters.

Thanks for this, I wasn't familiar with %q here!

DavidNix · 2022-06-27T17:31:48Z

ibc/types.go

+// GetShell returns the shell that should be used when executing scripts.
+func (c ChainConfig) GetShell() string {
+	if c.Shell == "" {
+		return "sh"
+	}
+	return c.Shell
+}
+


I agree. The benefit is keeping ChainConfig simple.

DavidNix · 2022-06-27T17:40:38Z

chain/penumbra/penumbra_chain.go

+func (c *PenumbraChain) RecoverKey(ctx context.Context, name, mnemonic string) error {
+	return fmt.Errorf("RecoverKey not implemented for PenumbraChain")
+}


Yup, Penumbra is highly experimental right now.

DavidNix

Apologies, I did not mean to approve previously, just comment.

Hit approve by mistake.

chatton · 2022-06-28T05:24:40Z

@mark-rushakoff ready for another look, I've addressed your feedback, thanks for taking a look so quickly!

Tests seem to be failing but it looks like an environmental issue on the test host?

agouin · 2022-06-28T05:36:53Z

Hi @chatton, looking at the test failure logs:

tn.NodeJob undefined (type *ChainNode has no field or method NodeJob)

Earlier today, NodeJob was replaced by Exec in #173, so those calls will need to be updated in chain_node.go.

chatton · 2022-06-28T06:13:24Z

@agouin thanks for letting me know! I had missed that PR being merged.

mark-rushakoff

Thanks @chatton!

feat: adding support for RecoverKey in the chain interface. Adding …

cf2b9e6

…helper method which generates a user using a provided mnemonic.

chatton requested a review from a team as a code owner June 27, 2022 14:33

chatton requested review from DavidNix and removed request for a team June 27, 2022 14:33

chatton mentioned this pull request Jun 27, 2022

E2E Testing Spike cosmos/ibc-go#1583

Closed

33 tasks

chatton commented Jun 27, 2022

View reviewed changes

mark-rushakoff reviewed Jun 27, 2022

View reviewed changes

DavidNix previously approved these changes Jun 27, 2022

View reviewed changes

DavidNix reviewed Jun 27, 2022

View reviewed changes

DavidNix self-requested a review June 27, 2022 18:14

chore: addressed PR feedback

63b1592

chatton added 2 commits June 28, 2022 07:04

Merge branch 'strangelove-ventures:main' into main

1f2fbf6

chore: changed NodeJob to Exec in ChainNode

539168e

mark-rushakoff approved these changes Jun 28, 2022

View reviewed changes

mark-rushakoff merged commit f3054ff into strangelove-ventures:main Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for `RecoverKey` in the chain interface. #176

Adding support for `RecoverKey` in the chain interface. #176

chatton commented Jun 27, 2022 •

edited

Loading

chatton Jun 27, 2022

mark-rushakoff Jun 27, 2022

DavidNix Jun 27, 2022

mark-rushakoff left a comment

mark-rushakoff Jun 27, 2022

mark-rushakoff Jun 27, 2022

chatton Jun 28, 2022

mark-rushakoff Jun 27, 2022

mark-rushakoff Jun 27, 2022

DavidNix Jun 27, 2022

mark-rushakoff Jun 27, 2022

mark-rushakoff Jun 27, 2022

mark-rushakoff Jun 27, 2022

mark-rushakoff Jun 27, 2022

mark-rushakoff Jun 27, 2022

chatton Jun 28, 2022

DavidNix Jun 27, 2022

DavidNix Jun 27, 2022

DavidNix left a comment

chatton commented Jun 28, 2022

agouin commented Jun 28, 2022

chatton commented Jun 28, 2022

mark-rushakoff left a comment

Adding support for RecoverKey in the chain interface. #176

Adding support for RecoverKey in the chain interface. #176

Conversation

chatton commented Jun 27, 2022 • edited Loading

Description

Checklist

Changes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-rushakoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidNix left a comment

Choose a reason for hiding this comment

chatton commented Jun 28, 2022

agouin commented Jun 28, 2022

chatton commented Jun 28, 2022

mark-rushakoff left a comment

Choose a reason for hiding this comment

Adding support for `RecoverKey` in the chain interface. #176

Adding support for `RecoverKey` in the chain interface. #176

chatton commented Jun 27, 2022 •

edited

Loading