Warp contract feature #724

morrisettathena · 2023-07-10T22:34:45Z

Why this should be merged

Extended warp e2e to use an example contract deployed to subnet A and B. Also fixed minor bug where both subnets used the same URI, causing them to have messages sent on the same network.

tests/warp/warp_test.go

Signed-off-by: morrisettjohn <60852062+morrisettjohn@users.noreply.github.com>

anusha-ctrl · 2023-07-11T15:37:08Z

tests/warp/warp_test.go


+		//deploy on subnetA
+		newHeads := make(chan *types.Header, 10)
+		sub, err := chainAWSClient.SubscribeNewHead(ctx, newHeads)


should we have a buffer of 10 for newHeads

anusha-ctrl · 2023-07-11T15:37:51Z

tests/warp/warp_test.go

@@ -169,16 +226,21 @@ var _ = ginkgo.Describe("[Warp]", ginkgo.Ordered, func() {
 		gomega.Expect(err).Should(gomega.BeNil())


On line 221 which err are we checking for?

None haha, bad typo

anusha-ctrl · 2023-07-11T15:39:11Z

tests/warp/warp_test.go

@@ -111,10 +119,13 @@ var _ = ginkgo.Describe("[Warp]", ginkgo.Ordered, func() {
 		blockchainIDA, blockchainIDB   ids.ID
 		chainAURIs, chainBURIs         []string
 		chainAWSClient, chainBWSClient ethclient.Client
+		chainANonce, chainBNonce       = uint64(0), uint64(0)
+		chainAExAddr, chainBExAddr     common.Address


what are the use cases of these nonces? I see them being incremented but not really used anywhere else

Mostly, those are used across different ginkgo tests. But, I think it might be better to fetch the nonce via the ethclient

Can we switch to using ethclient to fetch the correct nonce where they are needed?

If a nonce for the same address is used more than once in an It node, we should store it in a local variable and increment it after the first transaction.

Sounds good to me!

anusha-ctrl · 2023-07-11T15:40:34Z

tests/warp/warp_test.go

-			Nonce:     0,
-			To:        &warp.Module.Address,
+			Nonce:     1,
+			To:        &chainAExAddr,


why is this nonce hardcoded to 1?

It's probably a bad idea to have it hardcoded, I think it would probably be better to fetch the nonce via the ethclient, like above

richardpringle · 2023-07-11T20:18:25Z

contracts/contracts/ExampleWarp.sol

@@ -36,7 +35,7 @@ contract ExampleWarp {
    }

    // validateGetBlockchainID checks that the blockchainID returned by warp matches the argument
-    function validateGetBlockchainID(blockchainID bytes32) external {
+    function validateGetBlockchainID(bytes32 blockchainID) external view {


@aaronbuchwald, what would you do without @morrisettjohn?

I just hope he doesn't go back to school 😅. We can give him a degree right?

Haha that would be cool!

richardpringle · 2023-07-11T20:20:04Z

tests/warp/warp_test.go

+var (
+	WarpExampleRawABI string = `[{"inputs":[{"internalType":"bytes32","name":"destinationChainID","type":"bytes32"},{"internalType":"bytes32","name":"destinationAddress","type":"bytes32"},{"internalType":"bytes","name":"payload","type":"bytes"}],"name":"sendWarpMessage","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"expectedBlockchainID","type":"bytes32"}],"name":"validateGetBlockchainID","outputs":[],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"originChainID","type":"bytes32"},{"internalType":"bytes32","name":"originSenderAddress","type":"bytes32"},{"internalType":"bytes32","name":"destinationChainID","type":"bytes32"},{"internalType":"bytes32","name":"destinationAddress","type":"bytes32"},{"internalType":"bytes","name":"payload","type":"bytes"}],"name":"validateWarpMessage","outputs":[],"stateMutability":"view","type":"function"}]`
+	WarpExampleABI           = contract.ParseABI(WarpExampleRawABI)
+	WarpExampleBin    []byte = common.FromHex("60806040527302000000000000000000000000000000000000056000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff16021790555034801561006457600080fd5b506108db806100746000396000f3fe608060405234801561001057600080fd5b50600436106100415760003560e01c806315f0c959146100465780631d9a3246146100625780636a0b64dd1461007e575b600080fd5b610060600480360381019061005b919061033c565b61009a565b005b61007c600480360381019061007791906103ce565b61013e565b005b61009860048036038101906100939190610468565b61025b565b005b60008060009054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16634213cf786040518163ffffffff1660e01b8152600401602060405180830381865afa158015610108573d6000803e3d6000fd5b505050506040513d601f19601f8201168201806040525081019061012c91906104f1565b905081811461013a57600080fd5b5050565b60008060008054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16631f9b40ec6040518163ffffffff1660e01b8152600401600060405180830381865afa1580156101ac573d6000803e3d6000fd5b505050506040513d6000823e3d601f19601f820116820180604052508101906101d59190610764565b91509150806101e357600080fd5b878260000151146101f357600080fd5b8682602001511461020357600080fd5b8582604001511461021357600080fd5b8482606001511461022357600080fd5b83836040516102339291906107ff565b60405180910390208260800151805190602001201461025157600080fd5b5050505050505050565b60008054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16636a0b64dd858585856040518563ffffffff1660e01b81526004016102ba9493929190610865565b600060405180830381600087803b1580156102d457600080fd5b505af11580156102e8573d6000803e3d6000fd5b5050505050505050565b6000604051905090565b600080fd5b600080fd5b6000819050919050565b61031981610306565b811461032457600080fd5b50565b60008135905061033681610310565b92915050565b600060208284031215610352576103516102fc565b5b600061036084828501610327565b91505092915050565b600080fd5b600080fd5b600080fd5b60008083601f84011261038e5761038d610369565b5b8235905067ffffffffffffffff8111156103ab576103aa61036e565b5b6020830191508360018202830111156103c7576103c6610373565b5b9250929050565b60008060008060008060a087890312156103eb576103ea6102fc565b5b60006103f989828a01610327565b965050602061040a89828a01610327565b955050604061041b89828a01610327565b945050606061042c89828a01610327565b935050608087013567ffffffffffffffff81111561044d5761044c610301565b5b61045989828a01610378565b92509250509295509295509295565b60008060008060608587031215610482576104816102fc565b5b600061049087828801610327565b94505060206104a187828801610327565b935050604085013567ffffffffffffffff8111156104c2576104c1610301565b5b6104ce87828801610378565b925092505092959194509250565b6000815190506104eb81610310565b92915050565b600060208284031215610507576105066102fc565b5b6000610515848285016104dc565b91505092915050565b600080fd5b6000601f19601f8301169050919050565b7f4e487b7100000000000000000000000000000000000000000000000000000000600052604160045260246000fd5b61056c82610523565b810181811067ffffffffffffffff8211171561058b5761058a610534565b5b80604052505050565b600061059e6102f2565b90506105aa8282610563565b919050565b600080fd5b600080fd5b600067ffffffffffffffff8211156105d4576105d3610534565b5b6105dd82610523565b9050602081019050919050565b60005b838110156106085780820151818401526020810190506105ed565b60008484015250505050565b6000610627610622846105b9565b610594565b905082815260208101848484011115610643576106426105b4565b5b61064e8482856105ea565b509392505050565b600082601f83011261066b5761066a610369565b5b815161067b848260208601610614565b91505092915050565b600060a0828403121561069a5761069961051e565b5b6106a460a0610594565b905060006106b4848285016104dc565b60008301525060206106c8848285016104dc565b60208301525060406106dc848285016104dc565b60408301525060606106f0848285016104dc565b606083015250608082015167ffffffffffffffff811115610714576107136105af565b5b61072084828501610656565b60808301525092915050565b60008115159050919050565b6107418161072c565b811461074c57600080fd5b50565b60008151905061075e81610738565b92915050565b6000806040838503121561077b5761077a6102fc565b5b600083015167ffffffffffffffff81111561079957610798610301565b5b6107a585828601610684565b92505060206107b68582860161074f565b9150509250929050565b600081905092915050565b82818337600083830152505050565b60006107e683856107c0565b93506107f38385846107cb565b82840190509392505050565b600061080c8284866107da565b91508190509392505050565b61082181610306565b82525050565b600082825260208201905092915050565b60006108448385610827565b93506108518385846107cb565b61085a83610523565b840190509392505050565b600060608201905061087a6000830187610818565b6108876020830186610818565b818103604083015261089a818486610838565b90509594505050505056fea2646970667358221220b2cefd832fa6ca66aff8dba2e446584042974aa24a3f54e0f99e83fc9cf34edf64736f6c63430008140033")


Is there a better way to do this rather than having a massive line like this?

Previously, I used a goembed function, keeping the abi and bin in a separate folder. Aaron suggested it might be better to keep the data directly in this file. In other files contract abi/bins have been stored directly like this, so I think we're mostly trying to adhere to the same style (though those have mostly been much shorter)

I think we should embed this.

I think it makes everything a little cleaner, I added it back in

aaronbuchwald · 2023-07-17T13:20:56Z

tests/warp/warp_test.go

@@ -39,6 +41,12 @@ var (
 	manager = runner.NewNetworkManager(config)
 )

+var (


Could we move these local variables into the ginkgo.Describe var block so that we put all of the variables needed throughout the test there rather than using a global variable here?

imo we could either put all of the vars there or here, but we should put them in one place.

Agreed, it will definitely make it a lot cleaner

aaronbuchwald · 2023-07-17T13:21:53Z

tests/warp/warp_test.go


+		//deploy on subnetA


Suggested change

//deploy on subnetA

// deploy warp contracts on subnetA

we should add one character of whitespace between // and the actual comment

Sounds good!

aaronbuchwald · 2023-07-17T13:22:36Z

tests/warp/warp_test.go

+		gomega.Expect(recp.Status).Should(gomega.Equal(types.ReceiptStatusSuccessful)) //make sure status code is 1, contract deployed successfully
+		sub.Unsubscribe()
+
+		//deploy on subnetB


same whitespace comment here

Since this is the same code to deploy to both chains, could we change this to a for loop over the URIs or respective client values, and deploy the contract once to each so that we reduce the code duplication

That makes sense, can do

aaronbuchwald · 2023-07-17T13:24:50Z

tests/warp/warp_test.go

+		toKey, err := crypto.GenerateKey()
+		gomega.Expect(err).Should(gomega.BeNil())
+		sendToAddress = crypto.PubkeyToAddress(toKey.PublicKey)
+


We never actually use toKey because we only need an address value to include in the contract arguments here and not the key itself.

Could we use a hardcoded address here instead ie. common.HexToAddress("095e7baea6a6c7c4c2dfeb977efac326af552d87") so that we don't need this extra code

I just left it as a ginkgo test variable

aaronbuchwald · 2023-07-17T13:25:12Z

tests/warp/warp_test.go

@@ -336,12 +399,20 @@ var _ = ginkgo.Describe("[Warp]", ginkgo.Ordered, func() {
 		log.Info("Transaction2 triggered new block", "blockHash", newHead.Hash())
 		nonce++

-		packedInput, err := warp.PackGetVerifiedWarpMessage()
+		//attempt to send a warp message with no sigs, which should fail


same whitespace comment here

aaronbuchwald · 2023-07-17T13:25:19Z

tests/warp/warp_test.go

+		packedInput, err := WarpExampleABI.Pack(
+			"validateWarpMessage",
+			blockchainIDA,
+			chainAExAddr.Hash(), //calling address is the deployed exampleWarpTx, not the original funded address


same whitespace comment here

aaronbuchwald · 2023-07-17T13:25:37Z

tests/warp/warp_test.go

+		<-newHeads
+		recp, err := chainAWSClient.TransactionReceipt(ctx, signed.Hash())
+		gomega.Expect(err).Should(gomega.BeNil())
+		gomega.Expect(recp.Status).Should(gomega.Equal(types.ReceiptStatusSuccessful)) //make sure status code is 1, contract deployed successfully


same whitespace comment

aaronbuchwald · 2023-07-17T13:27:02Z

tests/warp/warp_test.go

+
+		log.Info("Waiting for contract creation on chain A to be accepted")
+		<-newHeads
+		recp, err := chainAWSClient.TransactionReceipt(ctx, signed.Hash())


nit: txReceipt. We should use as clear variable names as possible

aaronbuchwald

Can we make this PR add a test case for interacting with the warp precompile via contract rather than replacing the existing direct interaction via EOA?

morrisettathena · 2023-07-17T14:17:42Z

Can we make this PR add a test case for interacting with the warp precompile via contract rather than replacing the existing direct interaction via EOA?

Sounds good!

morrisettathena · 2023-07-18T19:26:05Z

closed for now in favor of https://github.com/ava-labs/subnet-evm/compare/warp-contract...warp-contract-testing?expand=1, as I had to make sure my commits were all signed

* fix test generator (#724) * add go mod tidy check

Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Co-authored-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com> Co-authored-by: Ceyhun Onur <ceyhun.onur@avalabs.org> Co-authored-by: Richard Pringle <richard.pringle@avalabs.org> Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com> Co-authored-by: Meaghan FitzGerald <meag.fitz@avalabs.org> Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Co-authored-by: mountcount <166301065+mountcount@users.noreply.github.com> Co-authored-by: chuangjinglu <chuangjinglu@outlook.com> Co-authored-by: guqicun <guqicun@outlook.com> Co-authored-by: Dmytrol <46675332+Dimitrolito@users.noreply.github.com> fix problematic const name and some typos in comment (#1400) fix some function names in interface comment (#1397) close database on shutdown (#1403) fix some function names in comment (#1405) Fix Typos in Documentation (#1404) fix test generator (#724)

morrisettathena added 4 commits July 10, 2023 18:19

extended warp e2e test

6ca7924

extended warp e2e test

dd89648

fixed nits

1e49479

fixed nits

5fbe68b

morrisettathena requested review from anusha-ctrl, ceyonur, darioush and aaronbuchwald as code owners July 10, 2023 22:34

morrisettathena commented Jul 11, 2023

View reviewed changes

tests/warp/warp_test.go Outdated Show resolved Hide resolved

morrisettathena commented Jul 11, 2023

View reviewed changes

tests/warp/warp_test.go Outdated Show resolved Hide resolved

Apply suggestions from code review

596b243

Signed-off-by: morrisettjohn <60852062+morrisettjohn@users.noreply.github.com>

anusha-ctrl reviewed Jul 11, 2023

View reviewed changes

richardpringle reviewed Jul 11, 2023

View reviewed changes

aaronbuchwald reviewed Jul 17, 2023

View reviewed changes

aaronbuchwald requested changes Jul 17, 2023

View reviewed changes

morrisettathena closed this Jul 18, 2023

ceyonur added a commit that referenced this pull request Jan 7, 2025

fix test generator (#724)

38e1ffb

ceyonur added a commit that referenced this pull request Jan 7, 2025

Coreth sync (#1418)

9d4ef1a

* fix test generator (#724) * add go mod tidy check

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warp contract feature #724

Warp contract feature #724

morrisettathena commented Jul 10, 2023

anusha-ctrl Jul 11, 2023

anusha-ctrl Jul 11, 2023

morrisettathena Jul 17, 2023

anusha-ctrl Jul 11, 2023

morrisettathena Jul 12, 2023

aaronbuchwald Jul 17, 2023

morrisettathena Jul 17, 2023

anusha-ctrl Jul 11, 2023

morrisettathena Jul 12, 2023

richardpringle Jul 11, 2023

aaronbuchwald Jul 11, 2023

morrisettathena Jul 12, 2023

richardpringle Jul 11, 2023

morrisettathena Jul 12, 2023

darioush Jul 17, 2023

morrisettathena Jul 18, 2023

aaronbuchwald Jul 17, 2023

morrisettathena Jul 17, 2023

aaronbuchwald Jul 17, 2023

morrisettathena Jul 17, 2023

aaronbuchwald Jul 17, 2023

aaronbuchwald Jul 17, 2023

morrisettathena Jul 17, 2023

aaronbuchwald Jul 17, 2023

morrisettathena Jul 17, 2023

morrisettathena Jul 17, 2023

aaronbuchwald Jul 17, 2023

aaronbuchwald Jul 17, 2023

aaronbuchwald Jul 17, 2023

aaronbuchwald Jul 17, 2023

aaronbuchwald left a comment

morrisettathena commented Jul 17, 2023

morrisettathena commented Jul 18, 2023

		@@ -169,16 +226,21 @@ var _ = ginkgo.Describe("[Warp]", ginkgo.Ordered, func() {
		gomega.Expect(err).Should(gomega.BeNil())

Warp contract feature #724

Warp contract feature #724

Conversation

morrisettathena commented Jul 10, 2023

Why this should be merged

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

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

aaronbuchwald left a comment

Choose a reason for hiding this comment

morrisettathena commented Jul 17, 2023

morrisettathena commented Jul 18, 2023