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

chore: update alloy version to 9.0 #485

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

karen-sarkisyan
Copy link
Contributor

@karen-sarkisyan karen-sarkisyan commented Dec 31, 2024

This is my take on issue #484 . I tried to keep it as simple as possible without any rework of underlying structure.

Key changes:
alloy crates have been updated to latest version of 9.0

Some breaking changes are likely to be introduced because of change TransactionRequest -> OpTransactionRequest type for OPStack.

I invite anyone interested to review and regression test this PR, feedback is much appreciated. I'd be happy to fix whatever has to be fixed.

@wiz-a16z
Copy link

wiz-a16z bot commented Dec 31, 2024

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities 1 High 4 Medium
Data Finding Sensitive Data
Secret Finding Secrets
IaC Misconfiguration IaC Misconfigurations
Total 1 High 4 Medium

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@karen-sarkisyan karen-sarkisyan marked this pull request as ready for review December 31, 2024 16:48
Transaction {
inner: inner_tx,
deposit_nonce,
deposit_receipt_version: None,
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've left it as None for now. Or do we want to implement a check for Canyon fork in scope of this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should assume that Canyon has been activated for now as all the superchain chains have already gone through that hard fork. Alternatively we can add the canyon time to the config so we can be smart about what to set this field.

Either way, I think setting it to None as the default is probably not the right choice as Canyon is already enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I went the wrong way here.

I used op-alloy's Transaction struct that has deposit_nonce and deposit_receipt_version fields.
While deposit_receipt_version would be easy with known block number, I think I used a wrong nonce -- the correct one is in the receipt, not envelope. And I'm not sure how to get receipt's data here.

Maybe this is why you used the alloy's generic Transaction in the first place and we should stick with it? What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm it seems there actually isn't even a way to extract the correct deposit nonce anyway, which is pretty unfortunate. I guess we can leave as is for now.

Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

Looking good. Just one thought on the Canyon stuff.

Transaction {
inner: inner_tx,
deposit_nonce,
deposit_receipt_version: None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should assume that Canyon has been activated for now as all the superchain chains have already gone through that hard fork. Alternatively we can add the canyon time to the config so we can be smart about what to set this field.

Either way, I think setting it to None as the default is probably not the right choice as Canyon is already enabled.

opstack/src/spec.rs Show resolved Hide resolved
@ncitron ncitron merged commit e367fc3 into a16z:master Jan 8, 2025
4 of 6 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.

2 participants