Skip to content
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

[ETCM-1095] eip2930 receipt #1103

Merged
merged 2 commits into from
Sep 8, 2021
Merged

Conversation

strauss-m
Copy link
Contributor

Description

Introduce Receipt for Type 01 transactions

Proposed Solution

Type 01 receipt content is the same than legacy ones, but prefixed with the transaction type byte

@strauss-m strauss-m changed the title Feature/etcm 1095 eip2930 receipt [ETCM-1095] eip2930 receipt Aug 18, 2021
@strauss-m strauss-m force-pushed the feature/etcm-1095_eip2930_receipt branch 2 times, most recently from b72c25d to 9b04b42 Compare August 19, 2021 08:32
@strauss-m strauss-m marked this pull request as ready for review August 19, 2021 08:37
@strauss-m strauss-m force-pushed the feature/etcm-1095_eip2930_receipt branch 3 times, most recently from 2a33d1f to 770f806 Compare August 22, 2021 11:47
}

implicit class TransactionTypeValidator(val transactionType: Byte) extends AnyVal {
def isValidTransactionType: Boolean = transactionType >= MinAllowedType && transactionType <= MaxAllowedType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a big fan of implicit conversion if we can avoid it because it can be surprising where it is used. Also, currently only Type01 is allowed. Wouldn't it be more safe to just allow that for now ?

Or maybe we can move that to the encoding/networking part because the fact that a transaction type is encoded as a number from 0 to 0x7f is not really relevant in the domain. We can have transaction types as an enumeration to make it impossible to have an invalid transaction type in the domain part, and let the networking part make the conversion.

Copy link
Contributor Author

@strauss-m strauss-m Sep 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the TransactionType enum:

  • only check when decoding RLP data
  • easier validation (transactionTypeByte is one of enum values)

As for the validation not being currently restricted to type01, I got two choices:

  • current implementation: can accept un-handled transaction type, but don't need to change for new transaction
  • proposed implementation: only valid for supported transaction types, but need to be updated for every new transaction

Your version seems more safe.

src/main/scala/io/iohk/ethereum/domain/Transaction.scala Outdated Show resolved Hide resolved
src/test/scala/io/iohk/ethereum/ObjectGenerators.scala Outdated Show resolved Hide resolved
@strauss-m strauss-m force-pushed the feature/etcm-1095_eip2930_receipt branch from 771aa72 to b511815 Compare September 6, 2021 09:07
@strauss-m strauss-m force-pushed the feature/etcm-1095_eip2930_receipt branch from b511815 to 971c033 Compare September 6, 2021 15:34
@strauss-m strauss-m merged commit 2da90e4 into develop Sep 8, 2021
@strauss-m strauss-m deleted the feature/etcm-1095_eip2930_receipt branch September 8, 2021 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants