[ETCM-921] transaction with access list #1094
Conversation
| .fromBytes( | ||
| ByteString( | ||
| Hex.decode( | ||
| "c9519f4f2b30335884581971573fadf60c6204f59a911df35ee8a540456b266032f1e8e2c5dd761f9e4f88f41c8310aeaba26a8bfcdacfedfa12ec3862d3752101" |
There was a problem hiding this comment.
How do we know this value is correct? Is this specified somewhere in the EIP?
There was a problem hiding this comment.
Dom tooks those values directly from core-geth own tests.
See https://github.com/ethereum/go-ethereum/blob/master/core/types/transaction_test.go#L71
There was a problem hiding this comment.
Gotcha! It would probably make sense to add a github link to those tests for posterity in a comment (pointing to a concrete commit, rather than master, in case they get moved)
| case r: RLPList if r.items.isEmpty => AccessListItem(null, List.empty) | ||
|
|
||
| case RLPList(rlpAddress, rlpStorageKeys: RLPList) => | ||
| val address = rlpAddress.decodeAs[Address]("address ") |
There was a problem hiding this comment.
there's a space in "address "
There was a problem hiding this comment.
good catch, fixed
strauss-m
force-pushed
the
feature/etcm-921_transaction_with_access_list
branch
5 times, most recently
from
8217580 to
8e3dbef
Compare
| * @param prefixedRLPEncodeable the RLPEncodable to prefix with | ||
| */ | ||
| case class PrefixedRLPEncodable(prefix: Byte, prefixedRLPEncodeable: RLPEncodeable) extends RLPEncodeable { | ||
| require(prefix <= 0x07f, "prefix should be lower than 0x7f") |
There was a problem hiding this comment.
scala> 0x07f.toByte
val res0: Byte = 127
scala> 128.toByte <= 0x7f.toByte
val res3: Boolean = true
Looks like all Bytes are smaller than 0x7f so it makes more sense to check whether prefix is larger than 0
There was a problem hiding this comment.
good catch, I'll change the check and add proper testing
| def toTypedRLPEncodables: Seq[RLPEncodeable] = | ||
| encodables match { | ||
| case Seq() => Seq() | ||
| case Seq(RLPValue(v), rlpList: RLPList, tail @ _*) if v.length == 1 && v.head < 0x7f => |
There was a problem hiding this comment.
Do you think it would make sense not to repeat the magic 0x7f in multiple places but putting it for example in a wrapper value class for Byte?
There was a problem hiding this comment.
good idea, I've extracted the magic number and added the proposed wrapper
strauss-m
force-pushed
the
feature/etcm-921_transaction_with_access_list
branch
from
d54774a to
d161ae4
Compare
| * @param prefixedRLPEncodeable the RLPEncodable to prefix with | ||
| */ | ||
| case class PrefixedRLPEncodable(prefix: Byte, prefixedRLPEncodeable: RLPEncodeable) extends RLPEncodeable { | ||
| require(prefix >= 0, "prefix should be in the range [0; 0x7f]") |
There was a problem hiding this comment.
0x7f is the limit for two reasons:
- EIP-2718: TransactionType is a positive unsigned 8-bit number between 0 and 0x7f that represents the type of the transcation
- rlp encoding spec: for a single byte whose value is in the [0x00, 0x7f] range, that byte is its own RLP encoding
That's what allows us to consider a single RLPValue [0; 0x7f] as a single raw byte transaction type (and vice-versa)
Since Byte is a signed type, the range [0; 0x7f] check become a simple positive condition.
|
|
||
| case RLPList(rlpAddress, rlpStorageKeys: RLPList) => | ||
| val address = rlpAddress.decodeAs[Address]("address") | ||
| val storageKeys = fromRlpList[BigInt](rlpStorageKeys).toList |
There was a problem hiding this comment.
regarding this BigInt type - were you able to verify if this is good enough? maybe a block from ETC mainnet with a typed transaction and the access list populated?
There was a problem hiding this comment.
I didn't try to find one, but it feel like a good real-life test case until we connect to mainnet.
I'll give it a look.
Concerning the StorageKey type, I've seen both BigInt and UInt256 in mantis code, as well as Hash (byte[32]) in go-ethereum. Maybe we should introduce (in another ticket ?) a specific StorageKey type similar to the Address one, with a 32 bytes length
| case _ => throw new RuntimeException("Cannot decode SignedTransactions") | ||
| } | ||
| } | ||
|
|
||
| implicit class SignedTransactionRlpEncodableDec(val rlpEncodeable: RLPEncodeable) extends AnyVal { | ||
|
|
||
| // scalastyle:off method.length |
There was a problem hiding this comment.
is there a matching :on in the file? can't find it
There was a problem hiding this comment.
No, I was thinking that the style annotation was on a per-method scope.
Maybe the best way is to actually split the method ?
I have mixed feeling about this: on one hand, the method is really long, but on the other one, it's quite straite-forward to read, as the lenght is due to the case class formatting, not actual statements.
However, once we start adding new transactions, we'll need to split it, so maybe doing it now is a good time. Do you have any preference (split vs scalastyle:off) ?
| // TODO ReceiptSpec | ||
| // TODO decode string to case class | ||
| // TODO adjust runVM in ledger | ||
| // TODO align how geth and besu treat typed transactions when sent via rpc before magneto HF |
There was a problem hiding this comment.
maybe drop these? no need to have the plan in develop
strauss-m
force-pushed
the
feature/etcm-921_transaction_with_access_list
branch
from
11d736c to
46260ae
Compare
strauss-m
force-pushed
the
feature/etcm-921_transaction_with_access_list
branch
from
46260ae to
c52e95a
Compare
Make
SignedTransactionwrapTransactionto allowTypedTransactions.This PR is split from #1061
Included in this branch are:
Follow up tickets are expected: