feat: Add tests for execution grpc server and astria configs #17

bharath-123 · 2024-05-16T11:13:29Z

This PR adds the following:

Tests for astria execution grpc server
Tests for astria config
Abstraction of astria validation logic and unit tests for them.

joroshiba · 2024-05-28T17:30:57Z

.github/workflows/run-tests.yml

@@ -22,4 +22,4 @@ jobs:
      - name: Run tests
        run: go test -short ./...
        env:
-          GOOS: linux


nit: new line

joroshiba · 2024-05-28T17:31:08Z

consensus/clique/clique.go

@@ -474,6 +474,7 @@ func (c *Clique) verifySeal(snap *Snapshot, header *types.Header, parents []*typ
 		return err
 	}
 	if _, ok := snap.Signers[signer]; !ok {
+		fmt.Printf("Failing here")


joroshiba · 2024-05-28T17:38:24Z

grpc/execution/validation.go

+)
+
+// if sequencer tx is valid, then a unmarshalled ethereum transaction is returned. if not valid, nil is returned
+func (s *ExecutionServiceServerV1Alpha2) ValidateAndUnmarshalSequencerTx(tx *sequencerblockv1alpha1.RollupData) (*types.Transaction, error) {


I'm not sure this is best as a method on the execution server. Maybe best choice for now, but only thing it's ingesting is bridge config information. Perhaps it could just take those as parameters?

This also only validates deposit tx types. Perhaps those should be validated elsewhere similar to how we validate the eth txs elsewhere just build the deposit don't validate validate elsewhere? These could be follow up changes though

yeah i thought the same too. I think it makes sense to pass the bridge config

yeah i think we can refactor the validation logic in the future. It would be better to seperate out the unmarshall step and validate step. but then we end up unmarshalling twice for validation.

…lSequencerTx

noot · 2024-05-29T02:23:49Z

grpc/execution/validation.go

+)
+
+// if sequencer tx is valid, then a unmarshalled ethereum transaction is returned. if not valid, nil is returned
+func ValidateAndUnmarshalSequencerTx(tx *sequencerblockv1alpha1.RollupData, bridgeAddresses map[string]*params.AstriaBridgeAddressConfig, bridgeAllowedAssetIDs map[[32]byte]struct{}) (*types.Transaction, error) {


doesn't need to be exported - make this private

noot · 2024-05-29T02:25:57Z

grpc/execution/validation.go

+	"github.com/ethereum/go-ethereum/params"
+)
+
+// if sequencer tx is valid, then a unmarshalled ethereum transaction is returned. if not valid, nil is returned


Suggested change

// if sequencer tx is valid, then a unmarshalled ethereum transaction is returned. if not valid, nil is returned

// `validateAndUnmarshalSequencerTx` returns an ethereum transaction if the given rollup data bytes are valid.

maybe describe what the validation for deposits looks like also?

noot · 2024-05-29T02:27:19Z

grpc/execution/validation.go

+		bridgeAddress := string(deposit.BridgeAddress.GetInner())
+		bac, ok := bridgeAddresses[bridgeAddress]
+		if !ok {
+			log.Debug("ignoring deposit tx from unknown bridge", "bridgeAddress", bridgeAddress)


don't log in this function, just put the required error contents in the returned error, as the caller will log

noot · 2024-05-29T02:30:06Z

grpc/execution/test_utils.go

+	if err := n.Start(); err != nil {
+		t.Fatal("can't start node:", err)
+	}


why do we have to actually start the node here?

Since all the server tests require it to start, i have just abstracted it into one place. The validation_test doesn't need it and I can add a flag to only start it when needed.

noot · 2024-05-29T02:31:16Z

grpc/execution/server_test.go

+	conn, err := grpc.Dial(GrpcEndpointWithoutPrefix(n), grpc.WithTransportCredentials(insecure.NewCredentials()))
+	if err != nil {
+		t.Fatalf("Failed to dial gRPC: %v", err)
+	}


is it possible to not actually start the gRPC server but just call methods directly on serviceV1Alpha1? that seems like it would work?

That would work. I just thought that this would be a more solid integration test where we run all the grpc node setup logic too.

noot · 2024-05-29T02:33:19Z

grpc/execution/server_test.go

+	if err != nil {
+		t.Fatalf("GetGenesisInfo failed: %v", err)
+	}


would be good to use something like this: https://pkg.go.dev/github.com/stretchr/testify/require

then you can do:

Suggested change

if err != nil {

t.Fatalf("GetGenesisInfo failed: %v", err)

}

if err != nil {

t.Fatalf("GetGenesisInfo failed: %v", err)

}

Suggested change

if err != nil {

t.Fatalf("GetGenesisInfo failed: %v", err)

}

require.Nil(t, err, "GetGenesisInfo failed")

and for the equals checks, can use require.Equal(t, a, b)

noot · 2024-05-30T17:06:23Z

grpc/execution/server.go

-			txsToProcess = append(txsToProcess, ethTx)
+		unmarshalledTx, err := validateAndUnmarshalSequencerTx(tx, s.bridgeAddresses, s.bridgeAllowedAssetIDs)
+		if err != nil {
+			log.Error("failed to validate sequencer tx, ignoring", "tx", tx, "err", err)


Suggested change

log.Error("failed to validate sequencer tx, ignoring", "tx", tx, "err", err)

log.Debug("failed to validate sequencer tx, ignoring", "tx", tx, "err", err)

noot · 2024-05-30T17:07:09Z

grpc/execution/server_test.go

+	ethservice, serviceV1Alpha1 := setupExecutionService(t, 10)
+
+	genesisInfo, err := serviceV1Alpha1.GetGenesisInfo(context.Background(), &astriaPb.GetGenesisInfoRequest{})
+	require.Nil(t, err, "GetGenesisInfo failed: %v", err)


Suggested change

require.Nil(t, err, "GetGenesisInfo failed: %v", err)

require.Nil(t, err, "GetGenesisInfo failed")

don't need to log the error as it'll already get logged by require - same for other lines

didn't know that!

noot · 2024-05-30T17:22:04Z

grpc/execution/server_test.go

+			txs := []*types.Transaction{}
+			marshalledTxs := []*sequencerblockv1alpha1.RollupData{}
+			for i := 0; i < 5; i++ {
+				unsignedTx := types.NewTransaction(uint64(i), common.HexToAddress("0x9a9070028361F7AAbeB3f2F2Dc07F82C4a98A02a"), big.NewInt(1), params.TxGas, big.NewInt(params.InitialBaseFee*2), nil)


put the address as a var outside the loop

noot · 2024-05-30T17:23:49Z

grpc/execution/server_test.go

+	txs := []*types.Transaction{}
+	marshalledTxs := []*sequencerblockv1alpha1.RollupData{}
+	for i := 0; i < 5; i++ {
+		unsignedTx := types.NewTransaction(uint64(i), common.HexToAddress("0x9a9070028361F7AAbeB3f2F2Dc07F82C4a98A02a"), big.NewInt(1), params.TxGas, big.NewInt(params.InitialBaseFee*2), nil)


same here about the address

actually make it a global var since it seems to be used a lot of places

noot · 2024-05-30T17:25:29Z

grpc/execution/test_utils.go

+		Config: &config,
+		Alloc: core.GenesisAlloc{
+			testAddr:                         {Balance: testBalance},
+			params.BeaconRootsStorageAddress: {Balance: common.Big0, Code: common.Hex2Bytes("3373fffffffffffffffffffffffffffffffffffffffe14604457602036146024575f5ffd5b620180005f350680545f35146037575f5ffd5b6201800001545f5260205ff35b6201800042064281555f359062018000015500")},


just curious, what is this code?

eip-4788 stuff, address where historical beacon roots are stored. but we can remove it. don't need it. i copied this over from another test.

noot · 2024-05-30T17:26:33Z

grpc/execution/test_utils.go

+	n, err := node.New(&node.Config{
+		P2P: p2p.Config{
+			ListenAddr:  "0.0.0.0:0",
+			NoDiscovery: true,
+			MaxPeers:    25,
+		},
+	})


is there a way to disable p2p? don't think we should start p2p in a unit test

removed it!

noot · 2024-05-30T17:26:42Z

grpc/execution/test_utils.go

+	if err != nil {
+		t.Fatal("can't create node:", err)
+	}


can use require, same for other places in this file

noot · 2024-05-30T17:27:35Z

grpc/execution/test_utils.go

+
+}
+
+func GrpcEndpointWithoutPrefix(n *node.Node) string {


remove (unused?)

noot · 2024-05-30T18:20:16Z

grpc/execution/validation_test.go

+	"testing"
+)
+
+func randomBlobTx() *types.Transaction {


Suggested change

func randomBlobTx() *types.Transaction {

func testBlobTx() *types.Transaction {

since it isn't random, same with randomDepositTx below

noot

looks good! only minor suggestions :)

bharath-123 changed the base branch from main to bharath/fix-existing-tests May 16, 2024 11:13

bharath-123 force-pushed the bharath/add-execution-server-tests branch 2 times, most recently from def1207 to a0190ef Compare May 16, 2024 11:17

bharath-123 changed the title ~~feat: Add tests for execution grpc server~~ feat: Add integration tests for execution grpc server May 16, 2024

bharath-123 changed the title ~~feat: Add integration tests for execution grpc server~~ feat: Add tests for execution grpc server May 16, 2024

bharath-123 marked this pull request as ready for review May 16, 2024 11:20

bharath-123 requested review from joroshiba, mycodecrafting and noot as code owners May 16, 2024 11:20

bharath-123 changed the title ~~feat: Add tests for execution grpc server~~ feat: Add tests for execution grpc server and configs May 20, 2024

bharath-123 changed the title ~~feat: Add tests for execution grpc server and configs~~ feat: Add tests for execution grpc server and astria configs May 20, 2024

bharath-123 force-pushed the bharath/add-execution-server-tests branch from 99ca51f to b10c667 Compare May 22, 2024 05:56

bharath-123 force-pushed the bharath/fix-existing-tests branch from 9bd89b4 to bb5bc7f Compare May 22, 2024 05:57

bharath-123 force-pushed the bharath/add-execution-server-tests branch from b10c667 to 3c13860 Compare May 22, 2024 06:03

bharath-123 force-pushed the bharath/fix-existing-tests branch from bb5bc7f to 6ae7856 Compare May 22, 2024 17:08

Base automatically changed from bharath/fix-existing-tests to main May 24, 2024 09:16

bharath-123 added 10 commits May 27, 2024 21:00

fix tests

dd68b8e

add github workflow to run tests

34fd089

fix CI file

80058a9

remove log

f96d50d

add unit tests for execution grpc server

feccaf1

add test case for deposit tx

dd3e382

update tests

3c23c83

add tests for astria bridge address config validation

f1523a1

abstract out sequencer tx validation

9fe377c

remove block check in get genesis info test

1a4b890

bharath-123 force-pushed the bharath/add-execution-server-tests branch from 3c13860 to 1a4b890 Compare May 27, 2024 15:59

joroshiba reviewed May 28, 2024

View reviewed changes

bharath-123 added 2 commits May 29, 2024 00:20

refactor tests to make them more readable

4784e07

remove stray print stmt

ad553f4

bharath-123 added 2 commits May 29, 2024 00:30

take in bridge addresses and allowed asset ids in ValidateAndUnmarsha…

641cc3c

…lSequencerTx

fix minor nit

b8b9cd9

noot reviewed May 29, 2024

View reviewed changes

bharath-123 added 2 commits May 29, 2024 12:36

use require package to make assertions in tests

7f64d5f

avoid using grpc server

2273524

bharath-123 force-pushed the bharath/add-execution-server-tests branch from aa5dd01 to 2273524 Compare May 29, 2024 16:38

noot reviewed May 30, 2024

View reviewed changes

noot approved these changes May 30, 2024

View reviewed changes

address review comments

168d8a0

bharath-123 merged commit 9185612 into main May 31, 2024
2 checks passed

bharath-123 deleted the bharath/add-execution-server-tests branch May 31, 2024 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add tests for execution grpc server and astria configs #17

feat: Add tests for execution grpc server and astria configs #17

bharath-123 commented May 16, 2024 •

edited

Loading

joroshiba May 28, 2024

joroshiba May 28, 2024

joroshiba May 28, 2024

bharath-123 May 28, 2024

bharath-123 May 28, 2024 •

edited

Loading

noot May 29, 2024

noot May 29, 2024 •

edited

Loading

noot May 29, 2024

noot May 29, 2024

bharath-123 May 29, 2024

noot May 29, 2024

bharath-123 May 29, 2024

noot May 29, 2024

noot May 30, 2024

noot May 30, 2024

bharath-123 May 31, 2024

noot May 30, 2024

noot May 30, 2024

noot May 30, 2024

bharath-123 May 31, 2024

noot May 30, 2024

bharath-123 May 31, 2024

noot May 30, 2024

bharath-123 May 31, 2024

noot May 30, 2024 •

edited

Loading

noot May 30, 2024

noot May 30, 2024

noot left a comment

	// if sequencer tx is valid, then a unmarshalled ethereum transaction is returned. if not valid, nil is returned
	// `validateAndUnmarshalSequencerTx` returns an ethereum transaction if the given rollup data bytes are valid.

	log.Error("failed to validate sequencer tx, ignoring", "tx", tx, "err", err)
	log.Debug("failed to validate sequencer tx, ignoring", "tx", tx, "err", err)

	require.Nil(t, err, "GetGenesisInfo failed: %v", err)
	require.Nil(t, err, "GetGenesisInfo failed")

	func randomBlobTx() *types.Transaction {
	func testBlobTx() *types.Transaction {

feat: Add tests for execution grpc server and astria configs #17

feat: Add tests for execution grpc server and astria configs #17

Conversation

bharath-123 commented May 16, 2024 • edited Loading

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

bharath-123 May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot May 29, 2024 • edited Loading

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

noot May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot left a comment

Choose a reason for hiding this comment

bharath-123 commented May 16, 2024 •

edited

Loading

bharath-123 May 28, 2024 •

edited

Loading

noot May 29, 2024 •

edited

Loading

noot May 30, 2024 •

edited

Loading