Add support for broadcasting arbitrary messages as test users #186

chatton · 2022-06-30T17:15:19Z

Description

This PR adds support for the ability to Broadcast transactions on CosmosChains

closes #185

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 new interface Broadcaster
Add Cosmos chain implementation.
Added NewLocalKeyringFromDockerContainer a mechanism to use the keychain of created test users.
Added test to ensure a broadcast with a created user succeeds (this also verifies NewLocalKeyringFromDockerContainer works as this would fail if the keyring was not there. )

Notes

I tried to design the interfaces in such a way so that the backing congiuration (tx.Factory and client.Context) are fully configurable by the caller so that we can change any arbitrary options we need.

mark-rushakoff

A couple high-level notes:

Move Broadcast interface into root ibctest package
cosmos_broadcaster.go can be named just broadcaster.go since it is already in the cosmos directory

I think the Broadcast interface is fine to focus on Cosmos, but others on the team have given Cosmos vs non-Cosmos more thought than I have, so it would be good to hear from someone else.

mark-rushakoff · 2022-07-01T13:34:57Z

broadcast/broadcast.go

@@ -0,0 +1,59 @@
+package broadcast


I think the types here make more sense in the root ibctest package.

mark-rushakoff · 2022-07-01T13:35:42Z

broadcast/broadcast.go

+
+type FactoryOpt func(factory tx.Factory) tx.Factory
+
+type User interface {


After moving to ibctest, this interface can be named BroadcastUser to distinguish from the User struct.

if we are in the ibctest package I believe we may just be able to accept an *ibctest.User directly. However maybe it is no harm to keep the interface.

mark-rushakoff · 2022-07-01T13:35:59Z

broadcast/broadcast.go

+
+// Tx uses the provided Broadcaster to broadcast all the provided messages which will be signed
+// by the User provided. The sdk.TxResponse and an error are returned.
+func Tx(ctx context.Context, broadcaster Broadcaster, broadcastingUser User, msgs ...sdk.Msg) (sdk.TxResponse, error) {


And this function can be named BroadcastTx.

mark-rushakoff · 2022-07-01T14:05:02Z

chain/cosmos/cosmos_broadcaster.go

+// defaultClientContext returns a default client context configured with the user as the sender.
+func (b *Broadcaster) defaultClientContext(fromUser broadcast.User, sdkAdd sdk.AccAddress) client.Context {
+	// initialize a clean buffer each time
+	b.buf = &bytes.Buffer{}


This should probably be b.buf.Reset().

internal/dockerutil/keyring.go

mark-rushakoff · 2022-07-01T14:18:55Z

internal/dockerutil/keyring.go

+		splitName := strings.Split(name, "/")
+
+		extractedFileName := splitName[len(splitName)-1]
+		isDirectory := extractedFileName == ""
+		if isDirectory {
+			continue
+		}


This could use isDirectory := strings.HasSuffix(name, "/") and extractedFileName := path.Base(name).

much cleaner, thanks for the suggestion!

chatton · 2022-07-01T15:21:52Z

@mark-rushakoff moving the code into the ibctest package results in a circular import between ibctest and cosmos with a call to cosmos.NewCosmosChain in chainfactory.go. What would be your preferred way of resolving this?

Some options are:

Create an additional package just to hold types
Remove broadcaster interface and just use the concrete cosmos.Broadcaster and cosmos.BroadCastUser
Maintain separate package for broadcast interfaces and types.

chatton · 2022-07-05T11:28:32Z

I ended up keeping the interfaces in a broadcast package, but moved the main function to ibctest as BroadcastTx

mark-rushakoff

Looks like this came together nicely. Thanks for figuring out a workaround for the import cycle I inadvertently suggested last week. I left a few comments on some minor style points, and then I think this can be merged.

mark-rushakoff · 2022-07-05T14:11:02Z

interchain_test.go

+		require.Equal(t, uint32(0), resp.Code)
+		require.NotEmpty(t, resp.Data)
+		require.NotEmpty(t, resp.TxHash)
+		require.NotEmpty(t, resp.Events)


Can this test also have an assertion that the user on gaia1 has received transferAmount?

mark-rushakoff · 2022-07-05T14:12:01Z

internal/dockerutil/keyring.go

+		return nil, err
+	}
+
+	if err := os.Mkdir(fmt.Sprintf("%s/keyring-test", localDirectory), os.ModePerm); err != nil {


Since this is working on a local filesystem, it should use filepath.Join(localDirectory, "keyring-test") instead of fmt.Sprintf with a literal /.

mark-rushakoff · 2022-07-05T14:12:29Z

internal/dockerutil/keyring.go

+			continue
+		}
+
+		filePath := fmt.Sprintf("%s/keyring-test/%s", localDirectory, extractedFileName)


filepath.Join here too.

internal/dockerutil/keyring.go

mark-rushakoff · 2022-07-05T14:27:31Z

chain/cosmos/broadcaster.go

+}
+
+// GetFactory returns an instance of tx.Factory that is configured with this Broadcaster's CosmosChain
+// and the provided user.


Wouldn't hurt to mention ConfigureFactoryOptions in the godoc for this method.

chain/cosmos/broadcaster.go

mark-rushakoff · 2022-07-05T14:29:36Z

chain/cosmos/broadcaster.go

+	_, ok := b.keyrings[user]
+	if !ok {
+		localDir := b.t.TempDir()
+		containerKeyringDir := fmt.Sprintf("%s/keyring-test", cn.HomeDir())


filepath.Join

mark-rushakoff · 2022-07-05T14:30:05Z

chain/cosmos/broadcaster.go

+// GetTxResponseBytes returns the sdk.TxResponse bytes which returned from broadcast.Tx.
+func (b *Broadcaster) GetTxResponseBytes(ctx context.Context, user broadcast.User) ([]byte, error) {
+	if b.buf == nil || b.buf.Len() == 0 {
+		return nil, fmt.Errorf("empty buffer, transaction has not be executed yet")


chatton · 2022-07-05T15:29:32Z

I've opened a separate PR to add support for the keyrings that we were discussing:

mark-rushakoff

Thanks!

chatton added 5 commits June 30, 2022 16:07

wip: Adding broadcast interface and cosmos chain broadcaster

8e3d1f4

chore: added additional docstrings

334194f

chore: cleaning up NewLocalKeyringFromDockerContainer function

1e9d3a4

chore: added test for broadcast

b175fbd

chore: reverted default values

f35ea95

chatton requested a review from a team as a code owner June 30, 2022 17:15

chatton requested review from mark-rushakoff and removed request for a team June 30, 2022 17:15

chatton added 2 commits July 1, 2022 10:07

chore: update keyring to use mapping instead of single instance

29be35c

chore: added wait in the create user tests

23a0c8e

mark-rushakoff reviewed Jul 1, 2022

View reviewed changes

chatton added 4 commits July 4, 2022 15:48

Merge remote-tracking branch 'upstream/main'

ea93673

fix: addressing PR feedback and updating to use official docker client

0a1c16c

chore: use buf.Reset()

eb2ddb9

addressing PR feedback

27bfe96

mark-rushakoff reviewed Jul 5, 2022

View reviewed changes

chatton added 2 commits July 6, 2022 09:26

chore: addressing PR comments

6edb295

chore: updating test to verify target account received funds

4ce4b0f

chatton mentioned this pull request Jul 6, 2022

E2E Testing Spike cosmos/ibc-go#1583

Closed

33 tasks

mark-rushakoff approved these changes Jul 6, 2022

View reviewed changes

mark-rushakoff merged commit c709d57 into strangelove-ventures:main Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for broadcasting arbitrary messages as test users #186

Add support for broadcasting arbitrary messages as test users #186

chatton commented Jun 30, 2022 •

edited

mark-rushakoff left a comment

mark-rushakoff Jul 1, 2022

mark-rushakoff Jul 1, 2022

chatton Jul 1, 2022 •

edited

mark-rushakoff Jul 1, 2022

mark-rushakoff Jul 1, 2022

mark-rushakoff Jul 1, 2022

chatton Jul 1, 2022

chatton commented Jul 1, 2022

chatton commented Jul 5, 2022

mark-rushakoff left a comment

mark-rushakoff Jul 5, 2022

mark-rushakoff Jul 5, 2022

mark-rushakoff Jul 5, 2022

mark-rushakoff Jul 5, 2022

mark-rushakoff Jul 5, 2022

mark-rushakoff Jul 5, 2022

chatton commented Jul 5, 2022

mark-rushakoff left a comment


		type FactoryOpt func(factory tx.Factory) tx.Factory

		type User interface {

Add support for broadcasting arbitrary messages as test users #186

Add support for broadcasting arbitrary messages as test users #186

Conversation

chatton commented Jun 30, 2022 • edited

Description

Checklist

Changes

Notes

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

chatton Jul 1, 2022 • edited

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

chatton commented Jul 1, 2022

chatton commented Jul 5, 2022

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

chatton commented Jul 5, 2022

mark-rushakoff left a comment

Choose a reason for hiding this comment

chatton commented Jun 30, 2022 •

edited

chatton Jul 1, 2022 •

edited