Break the unsigned transaction data out of the transaction #1640

tsachiherman · 2024-10-08T20:12:15Z

What ?

Break the existing Transaction struct into Transaction and an embedded struct TransactionData

Why ?

The usage of Transaction might not be sufficient to distinguish between the sign and the unsigned transaction.
The unsigned transaction ( i.e. TransactionData ) should be used only in the case of a new transaction being created, and before it's being signed.
The signed transaction should be used everywhere else. For that reason, the Transaction doesn't come with any constructor of it's own ( only marshaler/unmarshaler ).

joshua-kim · 2024-10-09T14:48:32Z

chain/signedtxn.go

I think it's actually easier to review this file if the struct definition is kept in transaction.go because so it shows up as a rename instead of a delete + add

joshua-kim · 2024-10-09T14:49:52Z

chain/signedtxn.go

+	stateKeys state.Keys
+}
+
+func (t *SignedTransaction) UnsignedTxnBytes() []byte { return t.Transaction.bytes }


nit: I prefer the receiver name being s since it's the first letter of the struct name.

Seems like we don't need this function since you can just call signedTx.UnsignedBytes() because Transaction is an embedded type.

Regarding the second one, the main advantage in having the UnsignedTxnBytes in SignedTransaction is that it doesn't need to return a potential error. The error case was already handled during the unmarshaler, so no error handling is required by the caller.

joshua-kim · 2024-10-09T15:28:46Z

chain/transaction.go

-	if len(t.unsignedBytes) > 0 {
-		return t.unsignedBytes, nil
+// Bytes returns the byte slice representation of the tx
+func (t *Transaction) Bytes() ([]byte, error) {


Seems to me like the diff in this function isn't needed

joshua-kim · 2024-10-09T15:30:50Z

chain/transaction.go

 	t.Base.Marshal(p)
-	if err := t.Actions.marshalInto(p); err != nil {
+	if err := p.Err(); err != nil {


I think this diff isn't needed since Packer will stop at the first error (ref).

I see.. so you're saying that adding this would have no practical functional impact since the packet would effectively be "blocked" for any new additions due to the error.

joshua-kim · 2024-10-09T15:33:47Z

chain/signedtxn.go

+	_ mempool.Item = (*SignedTransaction)(nil)
+)
+
+type SignedTransaction struct {


I'm not sure how I feel about this naming. I personally get pretty confused seeing SignedTransaction and Transaction as I keep assuming Transaction is signed. We could consider something else like SignedTransaction and UnsignedTransaction or Transaction and TransactionData.

It looks like we have only several places the use the unsigned transaction. Therefore, I'm not really opinionated here. I've changed the following:
SignedTransaction -> Transaction
Transaction -> TransactionData

aaronbuchwald

Few small nits and then this LGTM - thanks for cleaning this up, this makes it way easier to reason about the fields of a transaction that have been populated

aaronbuchwald · 2024-10-10T16:55:06Z

api/jsonrpc/client.go

@@ -170,8 +170,8 @@ func (cli *JSONRPCClient) GenerateTransactionManual(

 	// Build transaction
 	actionCodec, authCodec := parser.ActionCodec(), parser.AuthCodec()
-	tx := chain.NewTx(base, actions)
-	tx, err := tx.Sign(authFactory, actionCodec, authCodec)
+	unsignedTx := chain.NewTxnData(base, actions)


Can we change to NewTxData ? We typically use tx instead of txn

aaronbuchwald · 2024-10-10T16:55:59Z

tests/integration/integration.go

@@ -360,9 +360,12 @@ var _ = ginkgo.Describe("[Tx Processing]", ginkgo.Serial, func() {
 			require.NoError(err)
 			auth, err := authFactory.Sign(unsignedTxBytes)
 			require.NoError(err)
-			tx.Auth = auth
+			signedTxn := chain.Transaction{


Could we use signedTx instead of signedTxn ?

aaronbuchwald · 2024-10-10T16:56:30Z

vm/vm.go

-				continue
-			}
-			if err := tx.Auth.Verify(ctx, unsignedTxBytes); err != nil {
+			if err := tx.Verify(ctx); err != nil {


Can we update the function name to VerifyAuth so that it's explicit what's being verified vs. Execute and PreExecute ?

aaronbuchwald · 2024-10-10T16:57:45Z

chain/transaction_test.go

+	// call Bytes so that the "bytes" field would get populated.
+	_, err = txBeforeSign.UnsignedBytes()
+	require.NoError(err)


(not necessary for this PR) would it make sense to always create TransactionData via a single constructor, so that it's clear all of the fields are populated in one place?

aaronbuchwald · 2024-10-10T16:58:10Z

chain/transaction.go

@@ -28,28 +28,23 @@ var (
 	_ mempool.Item = (*Transaction)(nil)
 )

-type Transaction struct {
+type TransactionData struct {


❤️ makes this way easier to review

aaronbuchwald · 2024-10-10T17:05:43Z

chain/transaction.go


-func (t *Transaction) ID() ids.ID { return t.id }
+func (s *Transaction) Bytes() []byte { return s.bytes }


Can we change from using (s *Transaction) to (t *Transaction) to follow the convention of using the lowercase first letter of the type?

update

2601378

tsachiherman self-assigned this Oct 8, 2024

tsachiherman added 3 commits October 8, 2024 16:26

update

368a81d

update unit test.

59599e4

update docs.

1700ce5

tsachiherman marked this pull request as ready for review October 9, 2024 13:09

tsachiherman requested review from aaronbuchwald and joshua-kim as code owners October 9, 2024 13:09

joshua-kim reviewed Oct 9, 2024

View reviewed changes

tsachiherman added 5 commits October 10, 2024 09:25

apply requested changes.

bdd2347

update per cr

b996602

lint

a15112a

Merge branch 'main' into tsachi/signedtransaction

3d79837

Merge branch 'main' into tsachi/signedtransaction

d0cd932

tsachiherman enabled auto-merge (squash) October 10, 2024 13:48

tsachiherman added 2 commits October 10, 2024 09:55

update per cr

229d669

Merge branch 'main' into tsachi/signedtransaction

458e984

tsachiherman requested a review from joshua-kim October 10, 2024 16:39

tsachiherman changed the title ~~Create SignedTransaction out of the existing Transaction~~ Break the unsigned transaction data out of the transaction Oct 10, 2024

aaronbuchwald requested changes Oct 10, 2024

View reviewed changes

tsachiherman added 2 commits October 11, 2024 09:45

update per CR

0b64495

Merge branch 'main' into tsachi/signedtransaction

ba29d13

tsachiherman requested a review from aaronbuchwald October 11, 2024 14:33

aaronbuchwald approved these changes Oct 11, 2024

View reviewed changes

tsachiherman merged commit bca7739 into main Oct 11, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break the unsigned transaction data out of the transaction #1640

Break the unsigned transaction data out of the transaction #1640

tsachiherman commented Oct 8, 2024 •

edited

Loading

joshua-kim Oct 9, 2024

joshua-kim Oct 9, 2024

tsachiherman Oct 9, 2024

joshua-kim Oct 9, 2024

tsachiherman Oct 10, 2024

joshua-kim Oct 9, 2024

tsachiherman Oct 10, 2024

tsachiherman Oct 10, 2024

joshua-kim Oct 9, 2024

tsachiherman Oct 10, 2024

aaronbuchwald left a comment

aaronbuchwald Oct 10, 2024

aaronbuchwald Oct 10, 2024

aaronbuchwald Oct 10, 2024

aaronbuchwald Oct 10, 2024

aaronbuchwald Oct 10, 2024

aaronbuchwald Oct 10, 2024


		func (t *Transaction) ID() ids.ID { return t.id }
		func (s *Transaction) Bytes() []byte { return s.bytes }

Break the unsigned transaction data out of the transaction #1640

Break the unsigned transaction data out of the transaction #1640

Conversation

tsachiherman commented Oct 8, 2024 • edited Loading

What ?

Why ?

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

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

tsachiherman commented Oct 8, 2024 •

edited

Loading