Skip to content

Comments

Clean up tests and add a few new ones#197

Closed
smartcontracts wants to merge 28 commits intomasterfrom
YAS-540/contracts/consolidation
Closed

Clean up tests and add a few new ones#197
smartcontracts wants to merge 28 commits intomasterfrom
YAS-540/contracts/consolidation

Conversation

@smartcontracts
Copy link
Contributor

Description

Adds various tests where clearly missing (as per coverage reports) in preparation for an internal audit. Also cleans up some of the test helpers to clarify a few things.

Metadata

Fixes

Contributing Agreement

Copy link

@willmeister willmeister left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@K-Ho K-Ho left a comment

Choose a reason for hiding this comment

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

couple small nitpicks, but lgtm!

require(
stateManager.updatedStorageSlotCounter() == 0,
"There's still updated contracts to account for!"
stateManager.updatedContractsCounter() == 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


/* Internal Imports */
import {
DEFAULT_FORCE_INCLUSION_PERIOD,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

const isIncluded = await canonicalTxChain.verifyElement(
element,
wrongPosition,
position,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be wrongPosition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, whoops

Comment on lines +892 to +894
const elementInclusionProof = await localBatch.getElementInclusionProof(
elementIndex + 1 // Proof for the wrong thing
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this test is just trying to single out having invalid siblings, then maybe this proof should instead be

const elementInclusionProof = await localBatch.getElementInclusionProof(
        elementIndex // Proof for the right thing
      )
const wrongProof = await localBatch.getElementInclusionProof(
        elementIndex + 1 // Proof for the wrong thing
      )
elementInclusionProof.siblings = wrongProof.siblings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point here.

export const ZERO_UINT = '00'.repeat(32)
export const DEFAULT_FORCE_INCLUSION_PERIOD = 600

export const DEFAULT_UNSAFE_OPCODES = UNSAFE_OPCODES.concat([Opcode.CHAINID])
Copy link
Contributor

@K-Ho K-Ho Aug 4, 2020

Choose a reason for hiding this comment

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

Why is CHAINID concatenated here? Shouldn't we just add CHAINID to UNSAFE_OPCODES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UNSAFE_OPCODES comes from @eth-optimism/rollup-core. I didn't know if there was a reason why CHAINID wasn't part of UNSAFE_OPCODES there, didn't want to touch it.

@smartcontracts
Copy link
Contributor Author

Closing this because it's not worth the trouble to fix the merge conflicts.

@smartcontracts smartcontracts deleted the YAS-540/contracts/consolidation branch August 26, 2020 18:41
protolambda pushed a commit to protolambda/optimism that referenced this pull request May 1, 2022
theochap pushed a commit that referenced this pull request Dec 10, 2025
Hides the recursive variable in `TrieNode::insert` and `TrieNode::open`
theochap pushed a commit that referenced this pull request Jan 15, 2026
### Description

Ports the Span Batch Type to `op-alloy-protocol`. A follow-on pr will
implement batch validation and the prefix check.

### Provenance

Part of a port migrating the batch types from `kona-derive` to
`op-alloy`. See
[`kona#695`](op-rs/kona#695).
emhane added a commit that referenced this pull request Feb 3, 2026
Based on op-rs/op-reth#203 ,
op-rs/op-reth#204

This PR implements `StateProvider` given a `OpProofsStorage` instance.
It reads most data from the external database, but falls back on the
latest provider for block hashes and code by hash similar to the
existing historical provider in Reth.

This is an important part to implementing live syncing since we're
running the sync process on the DB being created.

In the Reth implementation, `proof.rs` is contained in `trie/db`, so I
think it makes sense to go in our DB crate. The `provider.rs` is in the
`reth-provider` crate, but I don't think a separate provider crate helps
us here, so I think we should also include that in the trie crate as
well.

---------

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
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