Wasm hooks E2E test & change intermediary address prefix #3937

nicolaslara · 2023-01-06T19:00:11Z

What is the purpose of the change

Adds an e2e test to wasm hooks

Brief Changelog

Adds an e2e test to wasm hooks

Testing and Verifying

att tests pass

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

czarcas7ic · 2023-01-06T23:00:10Z

tests/e2e/e2e_test.go

+	// co up two levels
+	projectDir := filepath.Dir(filepath.Dir(wd))
+	fmt.Println(wd, projectDir)


Suggested change

// co up two levels

projectDir := filepath.Dir(filepath.Dir(wd))

fmt.Println(wd, projectDir)

// go up two levels

projectDir := filepath.Dir(filepath.Dir(wd))

czarcas7ic · 2023-01-06T23:00:58Z

tests/e2e/e2e_test.go

+	// Using code_id 1 because this is the only contract right now. This may need to change if more contracts are added
+	contracts, err := nodeA.QueryContractsFromId(chainA.LatestCodeId)


I think this comment can be removed now right?

czarcas7ic · 2023-01-06T23:01:53Z

tests/e2e/e2e_test.go

+		if len(balance) == 0 {
+			return false
+		}
+		return balance[0].Amount.Int64() == 10


Where does 10 come from? Can this be not hard coded or does it make the most sense to do it this way?

I think this is from the ibs transfer call up above. +1 to have a variable that we can format into the ibc transfer command and use here

czarcas7ic · 2023-01-06T23:02:26Z

tests/e2e/e2e_test.go

+		amount := totalFunds.(map[string]interface{})["amount"].(string)
+		denom := totalFunds.(map[string]interface{})["denom"].(string)
+		// check if denom contains "uosmo"
+		return err == nil && amount == "10" && strings.Contains(denom, "ibc")


same question about 10

czarcas7ic · 2023-01-06T23:03:57Z

x/ibc-hooks/README.md

@@ -34,7 +34,7 @@ So we detail where we want to get each of these fields from:
 * Sender: We cannot trust the sender of an IBC packet, the counterparty chain has full ability to lie about it. 
 We cannot risk this sender being confused for a particular user or module address on Osmosis.
 So we replace the sender with an account to represent the sender prefixed by the channel and a wasm module prefix.
-This is done by setting the sender to `Bech32(Hash("ibc-memo-action" || channelID || sender))`, where the channelId is the channel id on the local chain. 
+This is done by setting the sender to `Bech32(Hash("ibc-wasm-hook-intermediaryg" || channelID || sender))`, where the channelId is the channel id on the local chain. 


I think there is a typo in "wasm-hook-intermediary" with the g at the end

czarcas7ic · 2023-01-06T23:04:46Z

x/ibc-hooks/client/cli/query.go

@@ -0,0 +1,56 @@
+package cli


can we use osmocli for this instead?

I started looking into that and realized I didn't know how to use osmocli 😅, so in the interest of time, I just went with what I knew

p0mvn

LGTM

p0mvn · 2023-01-07T03:46:59Z

tests/e2e/e2e_test.go

+	contractAddr := contracts[0]
+
+	validatorAddr := nodeB.GetWallet(initialization.ValidatorWalletName)
+	fmt.Println("validatorAddr", validatorAddr)


Suggested change

fmt.Println("validatorAddr", validatorAddr)

p0mvn · 2023-01-07T03:48:33Z

tests/e2e/e2e_test.go

+		if len(balance) == 0 {
+			return false
+		}
+		return balance[0].Amount.Int64() == 10


I think this is from the ibs transfer call up above. +1 to have a variable that we can format into the ibc transfer command and use here

p0mvn · 2023-01-07T03:54:04Z

tests/e2e/e2e_test.go

+
+	// sender wasm addr
+	senderBech32, err := ibchookskeeper.DeriveIntermediateSender("channel-0", validatorAddr, "osmo")
+	fmt.Println("sender", senderBech32)


nit: I'm not sure if prints are helpful for debugging purposes. If so, can we change them to be logs. Logs format lines and test case info, making it easier to debug and also don't bypass testing package's output buffering

Suggested change

fmt.Println("sender", senderBech32)

s.T().Log("sender", senderBech32)

prints were a leftover from when I was debugging. Can be removed (or logged)

p0mvn · 2023-01-07T03:54:27Z

tests/e2e/e2e_test.go

+		15*time.Second,
+		10*time.Millisecond,
+	)
+	fmt.Println("response", response)


Suggested change

fmt.Println("response", response)

s.T().Log("response", response)

* initial setup for manual and e2e tests * added cli for calculating the sender * tidy * added cleaner flags and better checks at the end * proper checks * cleanup * initial e2e tests * debugging * cleanup * tidy * remove import cycle * test cleanup * updated prefix * fix go test (cherry picked from commit 637f3f3) # Conflicts: # go.mod # go.sum # go.work.sum # tests/ibc-hooks/ibc_middleware_test.go # x/ibc-hooks/README.md # x/ibc-hooks/wasm_hook.go

* initial setup for manual and e2e tests * added cli for calculating the sender * tidy * added cleaner flags and better checks at the end * proper checks * cleanup * initial e2e tests * debugging * cleanup * tidy * remove import cycle * test cleanup * updated prefix * fix go test

* Add sender to wasm hooks (#3879) * added sender to wasm hooks * cleaner transfer. The module doesn't need to get the funds at any point * wasmhooks doesn't need an account anymore * using ibc hooks from this branch * added comment for account from when the upgrade happened * getting the prefix from the config * using address with prefix * updated ibc-hooks hash * updated sum files * updated spec * updaed go.mod to match main (bad merge) * removed unnecessary include * updated go.mod * updated go.mod * ibc-hooks mod change * external change * Wasm hooks E2E test & change intermediary address prefix (#3937) * initial setup for manual and e2e tests * added cli for calculating the sender * tidy * added cleaner flags and better checks at the end * proper checks * cleanup * initial e2e tests * debugging * cleanup * tidy * remove import cycle * test cleanup * updated prefix * fix go test * internal go mod change * external go mod change * go sum * document manual test script

nicolaslara added 12 commits January 6, 2023 12:10

initial setup for manual and e2e tests

5c044d2

added cli for calculating the sender

13020bc

tidy

f0afe3f

added cleaner flags and better checks at the end

16f63dd

proper checks

e3f105f

cleanup

c327aad

initial e2e tests

d1a910f

debugging

d284cee

cleanup

f15c22b

tidy

c86f771

remove import cycle

bdbb70f

test cleanup

bb6d103

github-actions bot added the C:CLI label Jan 6, 2023

updated prefix

7a9b24b

github-actions bot added the C:docs Improvements or additions to documentation label Jan 6, 2023

nicolaslara added V:state/breaking State machine breaking PR and removed C:docs Improvements or additions to documentation labels Jan 6, 2023

fix go test

26a269f

github-actions bot added the C:docs Improvements or additions to documentation label Jan 6, 2023

czarcas7ic approved these changes Jan 6, 2023

View reviewed changes

p0mvn approved these changes Jan 7, 2023

View reviewed changes

ValarDragon changed the title ~~Wasm hooks E2E test~~ Wasm hooks E2E test & change intermediary address prefix Jan 7, 2023

ValarDragon added the A:backport/v14.x backport patches to v14.x branch label Jan 7, 2023

ValarDragon approved these changes Jan 7, 2023

View reviewed changes

ValarDragon merged commit 637f3f3 into main Jan 7, 2023

ValarDragon deleted the nicolas/wasm-hooks-e2e branch January 7, 2023 08:53

mergify bot mentioned this pull request Jan 7, 2023

Wasm hooks E2E test & change intermediary address prefix (backport #3937) #3943

Closed

ValarDragon removed the A:backport/v14.x backport patches to v14.x branch label Jan 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wasm hooks E2E test & change intermediary address prefix #3937

Wasm hooks E2E test & change intermediary address prefix #3937

nicolaslara commented Jan 6, 2023

czarcas7ic Jan 6, 2023

czarcas7ic Jan 6, 2023

czarcas7ic Jan 6, 2023

p0mvn Jan 7, 2023

czarcas7ic Jan 6, 2023

czarcas7ic Jan 6, 2023

czarcas7ic Jan 6, 2023

nicolaslara Jan 7, 2023

p0mvn left a comment

p0mvn Jan 7, 2023

p0mvn Jan 7, 2023

p0mvn Jan 7, 2023

nicolaslara Jan 7, 2023

p0mvn Jan 7, 2023

		// Using code_id 1 because this is the only contract right now. This may need to change if more contracts are added
		contracts, err := nodeA.QueryContractsFromId(chainA.LatestCodeId)

	fmt.Println("sender", senderBech32)
	s.T().Log("sender", senderBech32)

	fmt.Println("response", response)
	s.T().Log("response", response)

Wasm hooks E2E test & change intermediary address prefix #3937

Wasm hooks E2E test & change intermediary address prefix #3937

Conversation

nicolaslara commented Jan 6, 2023

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

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

p0mvn 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