-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tipping tx subtype #260
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few cpmments.
overall - looking good
} | ||
|
||
func (tx *ArbitrumSubtypedTx) txType() byte { return ArbitrumSubtypedTxType } | ||
func (tx *ArbitrumSubtypedTx) TxSubtype() byte { return tx.TxData.txType() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I get it but I still find it kind of confusing and a room for error..
could we add a separate subtype field in SubtypedTx and use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to keep the subtype a constant, but adding a field invalidates the property
Maybe I should add a func (tx *ArbitrumSubtypedTx) SubTx() TxData
method, and then we'd use it this way: tx.SubTx().Type()
, would it be better?
@@ -244,6 +244,10 @@ func (tx *Transaction) decodeTyped(b []byte, arbParsing bool) (TxData, error) { | |||
var inner BlobTx | |||
err := rlp.DecodeBytes(b[1:], &inner) | |||
return &inner, err | |||
case ArbitrumSubtypedTxType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this belongs a few lines up - under
if arbParsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to parse ArbitrumSubtypedTxType
even when arbParsing
is false to be able to decode binary encoding with UnmarshalBinary
to support SendRawTransaction
and SendRawTransactionConditional
. If we don't want to support it, then we probably need to add custom RPC method to allow sending subtyped txes (?).
@@ -48,6 +48,7 @@ type txJSON struct { | |||
YParity *hexutil.Uint64 `json:"yParity,omitempty"` | |||
|
|||
// Arbitrum fields: | |||
Subtype *hexutil.Uint64 `json:"subtype,omitempty"` // ArbitrumSubtypedTx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why uint64 and not byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- to make it consistent with type of "type" field which is also
hexutil.Uint64
- there's no
hexutil.Byte
, andhexautil.Uint64
does all format checking and decoding from string
case *ArbitrumSubtypedTx: | ||
switch inner.TxData.(type) { | ||
case *ArbitrumTippingTx: | ||
V, R, S := tx.RawSignatureValues() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 options, I'm o.k. with any:
- add a comment that this comes from london signer as these are assentially DynamicFeeTxType
- create a fake DynamicFeeTx and use the internal signer, similar to ArbitrumLegacyTx
- separate the code that extract the sender inside london-signer (without checking tx type), call this func from both london signer after checking type and from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 - we can't make fake DynamicFeeTx
and pass it to londonSigner
because Hash
method must be called on arbitrumSigner
to include transaction subtype in hashed data
3 - probably would create some unnecessary merge conflicts against upstream
so I went with 1 and added a comment.
In SignatureValues
method I used the fake tx pattern, as there it was possible.
ArbitrumSubtypedTx
)ArbitrumTippingTx
)