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

optimism deposit transaction #5

Merged
merged 9 commits into from
Feb 28, 2025
Merged

Conversation

thinkAfCod
Copy link
Contributor

PR description

Added the deposit transaction type, along with the related encoders and decoders.

This was referenced Jan 22, 2025
* FlzCompressLen returns the length of the data after compression through FastLZ, based on <a
* href="">https://github.com/Vectorized/solady/blob/5315d937d79b335c668896d7533ac603adac5315/js/solady.js</a>
*/
static long flzCompressLength(final byte[] ib) {
Copy link

Choose a reason for hiding this comment

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

there is no library we can use for that ? or maybe push this class in an utility class or tuweni ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have only found implementations of FLZ compression in Netty and jfastlz, but they only provide encode/decode methods and do not offer a way to directly calculate the compressed length.

This flzCompressLength method is my Java implementation translated from the one in the op-alloy-flz repository.

It only calculates the length of the compressed data without performing the actual compression.

And it will only be used when creating an instance of RollGasData.

I think it might be more appropriate to move it to a utility class separately.

Copy link

@matkt matkt Jan 28, 2025

Choose a reason for hiding this comment

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

Yes I think we can move it to a utility class , thanks

@@ -28,6 +28,8 @@ public enum TransactionType {
EIP1559(0x02),
/** Blob transaction type. */
BLOB(0x03),
/** Optimism Deposit transaction type. */
OPTIMISM_DEPOSIT(0x7e),
Copy link

Choose a reason for hiding this comment

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

idk if it will be easy but I will prefer to have another transaction type list for optimism in order to see more easilty the type of transaction by network

Copy link

Choose a reason for hiding this comment

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

did you had time to look at this point ?

CodeDelegationTransactionDecoder::decode);
CodeDelegationTransactionDecoder::decode,
TransactionType.OPTIMISM_DEPOSIT,
OptimismDepositTransactionDecoder::decode);
Copy link

Choose a reason for hiding this comment

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

same here I will prefer to separate as much as possible mainnet code and layer 2 code. I think it will also simplify the next step when we will want to push this code in a plugin

Copy link

Choose a reason for hiding this comment

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

example OptimismTransactionDecoder implement TransactionDecoder
example MainnetTransactionDecoder implement TransactionDecoder

and something similar for the other classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created two new interfaces: TransactionEncoder.EncoderProvider and TransactionDecoder.DecoderProvider, which are used to obtain the encoder and decoder, respectively.

The encoder list and decoder list are maintained in OptimismTransactionEncoderDecoderProvider and MainnetTransactionEncoderDecoderProvider.

TransactionEncoder and TransactionDecoder use MainnetTransactionEncoderDecoderProvider by default.

@garyschulte
Copy link
Collaborator

If this unit test fails a 4th time, it likely is not a flaky test:

TransactionPoolOptionsTest > maxPrioritizedTxsPerTypeWrongTxType() FAILED
    java.lang.AssertionError at TransactionPoolOptionsTest.java:415

@thinkAfCod
Copy link
Contributor Author

I found it and I'm trying to fix the issue with the unit test failure.

Signed-off-by: thinkAfCod <[email protected]>
@thinkAfCod
Copy link
Contributor Author

This PR is valid now, could be merged.

Copy link

@matkt matkt left a comment

Choose a reason for hiding this comment

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

really like the last modification just asked one more question regarding the TransactionType enum

@@ -28,6 +28,8 @@ public enum TransactionType {
EIP1559(0x02),
/** Blob transaction type. */
BLOB(0x03),
/** Optimism Deposit transaction type. */
OPTIMISM_DEPOSIT(0x7e),
Copy link

Choose a reason for hiding this comment

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

did you had time to look at this point ?

@thinkAfCod
Copy link
Contributor Author

thinkAfCod commented Feb 24, 2025

Below is an example of the modification:

Extract TransactionType into an interface.

public interface TransactionType {

  boolean isFrontier();

  boolean isBlob();

  boolean isAccessList();

  boolean isDelegateCode();

  int getTypeValue();

  Set<TransactionType> getAccessListSupportedTransactionTypes();

  Set<TransactionType> getLegacyFeeMarketTransactionTypes();
}

TransactionType will implement this interface:

public enum MainnetTransactionType implements TransactionType {
  /** The Frontier. */
  FRONTIER(0xf8 /* this is serialized as 0x0 in TransactionCompleteResult */),
  /** Access list transaction type. */
  ACCESS_LIST(0x01),
  /** Eip1559 transaction type. */
  EIP1559(0x02),
  /** Blob transaction type. */
  BLOB(0x03),
  /** Eip7702 transaction type. */
  DELEGATE_CODE(0x04);
  
  // implement the TransactionTypeInfo interface...  
}

Also OptimismTransactionType will implement TransactionType interface too:

public enum OptimismTransactionType implements TransactionType {
  /** The Frontier. */
  FRONTIER(0xf8 /* this is serialized as 0x0 in TransactionCompleteResult */),
  /** Access list transaction type. */
  ACCESS_LIST(0x01),
  /** Eip1559 transaction type. */
  EIP1559(0x02),
  /** Blob transaction type. */
  BLOB(0x03),
  /** Eip7702 transaction type. */
  DELEGATE_CODE(0x04),
  /** Optimism Deposit transaction type. */
  OPTIMISM_DEPOSIT(0x7e);
  
  // implement the TransactionType interface...  
}

Since many places directly reference the FRONTIER enumeration type in MainnetTransactionType, it may be necessary to add an additional TransactionTypeProvider type to obtain the actual enumeration values instead of directly referencing the enumeration type.

Even so, there are probably over 100 files that need to be modified.

@thinkAfCod thinkAfCod force-pushed the deposit_tx branch 2 times, most recently from 991cfa6 to 21c0deb Compare February 27, 2025 02:47
@matkt matkt merged commit e0e67b1 into Consensys:main Feb 28, 2025
82 checks passed
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.

3 participants