chain: add Transaction JSON Marshal Unmarshal #1665

najeal · 2024-10-16T00:06:07Z

Few notes:

After a Marshal -> Unmarshal, the Transaction looses the Base data and internals like bytes, size, stateKeys.
The tests require the file to be part of chain_test package instead of chain, otherwise it ends up with import cycle (seems to come from auth package).
So the created test is outside the chain package and Transaction.id is unreachable. As a workaround I created a SetID function for testing purpose but it is not ideal. One other solution can be to make the id a public field.

There are also multiple options to avoid import cycle while testing in chain package:

Move Auth Rule Action Object Marshaler AuthFactory AuthbatchVerifier in a separate dedicated package.
Mock the objects coming from auth package used in the tests.

aaronbuchwald · 2024-10-23T19:38:16Z

chain/transaction.go

@@ -162,6 +163,8 @@ func (t *Transaction) Size() int { return t.size }

 func (t *Transaction) ID() ids.ID { return t.id }

+func (t *Transaction) SetID(id ids.ID) { t.id = id }


We should avoid exporting a function that overrides the ID of the transaction since it should always match the contents of the transaction

aaronbuchwald · 2024-10-23T19:39:01Z

chain/transaction.go

+		ID      ids.ID `json:"id"`
+		Actions []byte `json:"actions"`
+		Auth    []byte `json:"auth"`
+	}


Can we include Base here?

aaronbuchwald · 2024-10-23T19:42:26Z

chain/transaction.go

+		return nil, err
+	}
+
+	hexActions, err := codec.Bytes(actionsPacker.Bytes()).MarshalText()


We don't need to call MarshalText() on this. We can just pass in the codec.Bytes type directly and this will be handled in json.Marshal

aaronbuchwald · 2024-10-23T19:43:18Z

chain/transaction.go

+		Actions []byte `json:"actions"`
+		Auth    []byte `json:"auth"`


These types should be codec.Bytes so that marshalling them to hex is handled by json.Marshal

aaronbuchwald · 2024-10-23T19:43:41Z

chain/transaction.go

+	type txJSON struct {
+		ID      ids.ID `json:"id"`
+		Actions []byte `json:"actions"`
+		Auth    []byte `json:"auth"`
+	}


Can we move the type definition outside of these functions so we don't need to duplicate it?

aaronbuchwald · 2024-10-23T19:44:10Z

chain/transaction.go

+	var actionsBytes, authBytes codec.Bytes
+	err = actionsBytes.UnmarshalText(tx.Actions)
+	if err != nil {
+		return fmt.Errorf("%w: cannot unmarshal actions text", err)
+	}
+	err = authBytes.UnmarshalText(tx.Auth)
+	if err != nil {
+		return fmt.Errorf("%w: cannot unmarshal auth text", err)
+	}
+


Same comment for removing UnmarshalText in favor of using codec.Bytes in the txJSON type

aaronbuchwald · 2024-10-23T19:54:42Z

chain/transaction.go

+	t.id = tx.ID
+	t.TransactionData = TransactionData{
+		Actions: actions,
+	}
+	t.Auth = auth
+	return nil


Rather than trusting the received txID (which could be a mismatch), we should re-marshal the received tx based on the fields (Base, Actions, and Auth) and unmarshal it to populate the txID.

Ideally we can restrict the number of places where we construct a transaction, so that it's clear all of the required fields are always populated.

Right now the only place we create Transaction is in Sign. We could create a new function NewTx(base, actions, auth) that we call within Sign to keep it to a single place where we construct a signed transaction.

aaronbuchwald · 2024-10-23T19:57:58Z

chain/transaction_test.go

+	tx := &chain.Transaction{
+		TransactionData: chain.TransactionData{
+			Actions: []chain.Action{
+				&mockTransferAction{
+					To:    codec.Address{1, 2, 3, 4},
+					Value: 4,
+					Memo:  []byte("hello"),
+				},
+				&mockTransferAction{
+					To:    codec.Address{4, 5, 6, 7},
+					Value: 123,
+					Memo:  []byte("world"),
+				},
+				&action2{
+					A: 2,
+					B: 4,
+				},
+			},
+		},
+		Auth: &auth.ED25519{
+			Signer: pk.PublicKey(),
+		},
+	}


Could we use NewTxData and Sign to produce the signed transaction? This makes sure all fields are populated (currently missing the signature in the ED25519 auth)

…unction

…rshal

najeal

few remarks.

chain/transaction.go

najeal · 2024-10-28T16:50:03Z

chain/transaction_test.go

+	require.NoError(txout.Auth.Verify(context.Background(), prevUnsignedBytes))
+	require.NoError(txout.Auth.Verify(context.Background(), unsignedBytes))
+}
+
 // TestMarshalUnmarshal roughly validates that a transaction packs and unpacks correctly
 func TestMarshalUnmarshal(t *testing.T) {


@aaronbuchwald it looks like this test is incomplete. No unmarshal is done to verify we get back the same transaction data than the original one. I may open a issue to complete it in another PR?

Yup, feel free to open a PR directly (no need for an issue)

chain/transaction_test.go

aaronbuchwald · 2024-10-30T13:17:38Z

chain/transaction_test.go

+func TestJSONMarshalUnmarshal(t *testing.T) {
+	require := require.New(t)
+
+	txdata := chain.TransactionData{


nit: camel case to txData

separate from this PR, but we should be consistent with the variable name throughout this file as well

aaronbuchwald · 2024-10-30T13:19:32Z

chain/transaction_test.go

+	err = txout.UnmarshalJSON(b, parser)
+	require.NoError(err)
+	// cannot check direct Equal between signedTx and txout
+	// because of the Auth codec.Address that will be different after the unmarshaling.


Why is auth different after unmarshalling?

private field addr is not Marshalled, looking at the address() function is shows:

func (d *ED25519) address() codec.Address { if d.addr == codec.EmptyAddress { d.addr = NewED25519Address(d.Signer) } return d.addr }

I see the same logic for BLS, ED25519, SECP256R1.

The field by field check seems a bit brittle. Could we reduce this to a single check that the transaction unmarshalled from JSON produces the identical bytes as the original tx?

aaronbuchwald · 2024-10-30T13:20:25Z

chain/transaction_test.go

+
+	parser := chaintest.NewParser(nil, actionCodec, authCodec, nil)
+
+	var txout chain.Transaction


nit: rename to txFromJSON

aaronbuchwald · 2024-10-30T13:21:22Z

chain/transaction_test.go

+	require.Equal(signedTx.Base, txout.Base)
+	require.Equal(signedTx.Actions, txout.Actions)
+	require.Equal(signedTx.ID(), txout.ID())
+	require.Equal(signedTx.Bytes(), txout.Bytes())
+	require.Equal(signedTx.Size(), txout.Size())
+	// verify txout Auth is able to verify unsigned bytes of both original tx and unmarshaled tx.
+	prevUnsignedBytes, err := signedTx.UnsignedBytes()
+	require.NoError(err)
+	unsignedBytes, err := txout.UnsignedBytes()
+	require.NoError(err)
+	require.NoError(txout.Auth.Verify(context.Background(), prevUnsignedBytes))
+	require.NoError(txout.Auth.Verify(context.Background(), unsignedBytes))
+}


Rather than perform every check one by one, could we do a single require.Equal(signedTx, txFromJSON) ?

Unfortunatly, I can't because of the Auth.addr being different. I thought it was expected behavior.
The way I verified Auth correctness, I checked it can verify both the (prev/new)unsignedBytes.

chain/transaction.go

aaronbuchwald · 2024-10-30T13:35:03Z

chain/transaction.go

+	ID      ids.ID      `json:"id"`
+	Actions codec.Bytes `json:"actions"`
+	Auth    codec.Bytes `json:"auth"`
+	Base    codec.Bytes `json:"base"`


Could we keep Base as the original type since we should be able to encode/decode this directly as JSON with no issues?

aaronbuchwald · 2024-10-30T13:35:48Z

chain/transaction.go

+	if t.Base != nil {
+		t.Base.Marshal(basePacker)
+	}


What's the reason for this check? Base should never be nil.

it should not, but as it is a pointer I prefered to check it instead of risking a panic 🙂 but anyway we don't need to use the codec as you suggested in another comment 👍

Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: nathan haim <[email protected]>

aaronbuchwald · 2024-11-05T14:19:38Z

chain/transaction.go

+	p := codec.NewWriter(0, consts.NetworkSizeLimit)
+	if _, err := tx.UnsignedBytes(); err != nil {
+		return nil, err
+	}
+	if err := tx.Marshal(p); err != nil {
+		return nil, err
+	}
+	if err := p.Err(); err != nil {
+		return nil, err
+	}


Could we move creating the codec to directly before it's used and use the unsigned bytes to calculate how much memory to pre-allocate for the transaction (same as before this should be len(unsignedBytes) + consts.ByteLen + auth.Size) )

…rshal

chain: add Transaction JSON Marshal Unmarshal

215d3a9

najeal requested review from aaronbuchwald and joshua-kim as code owners October 16, 2024 00:06

aaronbuchwald requested changes Oct 23, 2024

View reviewed changes

najeal added 6 commits October 28, 2024 08:50

chain: tx marshaling use codec.Bytes as field type

e389ab9

chain: move txJSON type outside (un)marshal functions to avoid duplicate

4291264

chain: add Base to (un)marshalJSON and create a Transaction factory f…

6c1e28c

…unction

api: update use of tx Sign() method

e94dca6

Merge remote-tracking branch 'upstream/main' into transaction-json-ma…

bafaace

…rshal

chain: fix tests lint

9e3edfb

najeal commented Oct 28, 2024

View reviewed changes

aaronbuchwald requested changes Oct 30, 2024

View reviewed changes

najeal and others added 2 commits October 30, 2024 17:26

Apply suggestions from code review

61b17e7

Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: nathan haim <[email protected]>

chain: change json Base field type + fix nits

b49dce9

aaronbuchwald reviewed Nov 5, 2024

View reviewed changes

najeal added 2 commits November 5, 2024 23:12

chain: reduce test + init codec writer with accurate len

1b87eb5

Merge remote-tracking branch 'upstream/main' into transaction-json-ma…

567d85d

…rshal

aaronbuchwald approved these changes Nov 5, 2024

View reviewed changes

aaronbuchwald enabled auto-merge (squash) November 5, 2024 22:19

aaronbuchwald merged commit 3a6c0c2 into ava-labs:main Nov 5, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chain: add Transaction JSON Marshal Unmarshal #1665

chain: add Transaction JSON Marshal Unmarshal #1665

najeal commented Oct 16, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

aaronbuchwald Oct 23, 2024

najeal left a comment

najeal Oct 28, 2024

aaronbuchwald Nov 5, 2024

aaronbuchwald Oct 30, 2024

aaronbuchwald Oct 30, 2024

najeal Oct 30, 2024

aaronbuchwald Nov 5, 2024

aaronbuchwald Oct 30, 2024

aaronbuchwald Oct 30, 2024

najeal Oct 30, 2024

aaronbuchwald Oct 30, 2024

aaronbuchwald Oct 30, 2024

najeal Oct 30, 2024

aaronbuchwald Nov 5, 2024

		@@ -162,6 +163,8 @@ func (t *Transaction) Size() int { return t.size }

		func (t *Transaction) ID() ids.ID { return t.id }

		func (t *Transaction) SetID(id ids.ID) { t.id = id }


		parser := chaintest.NewParser(nil, actionCodec, authCodec, nil)

		var txout chain.Transaction

chain: add Transaction JSON Marshal Unmarshal #1665

chain: add Transaction JSON Marshal Unmarshal #1665

Conversation

najeal commented Oct 16, 2024

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

najeal 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