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

feat(client): providers generic over oracles #336

Merged
merged 8 commits into from
Jul 23, 2024

Conversation

zobront
Copy link
Contributor

@zobront zobront commented Jun 27, 2024

Description

This PR includes a range of changes that will make it easier for our zkvm-client (and likely future integrating teams) to work with kona-client directly rather than forking and modifying it.

Changes:

  • The CommsClient trait encompasses PreimageOracleClient + Clone + HintWriterClient. Therefore anything that implements this trait (which will be the Oracles) can both fetch and hint data.
  • Update CachingOracle to include the hint writer, and therefore properly implement CommsClient.
  • Providers perform all hinting and fetching via self.oracle (which implements CommsClient) and therefore never need to access the hint writer directly. This includes TrieDBHinter, which is now implemented on L2Provider rather than requiring the TrieDBHintWriter type.
  • A zkVM io type is added that is unimplemented. This doesn't do anything because we don't use the io system at all in the zkVM, but is required for compilation.
  • The PreimageKey type in the preimage crate now has an rkyv flag for serialization instead of serde. This is used for more efficient serialization that is better for us when passing data into the zkVM.

Tests

N/A

Additional context

N/A

Metadata

N/A

@zobront zobront changed the title feat(client): make providers generic over oracles feat(client): providers generic over oracles Jun 27, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.7%. Comparing base (a4ae5f5) to head (e44c42e).
Report is 19 commits behind head on main.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bin/client/src/kona.rs Outdated Show resolved Hide resolved
Comment on lines +60 to +63
let precompile_overrides = FPVMPrecompileOverride::<
OracleL2ChainProvider<CachingOracle>,
OracleL2ChainProvider<CachingOracle>,
>::default();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clabby I understand the logic for keeping a separate Fetcher and Hinter in the library crates, as we want to keep flexibility for future users to implement these separately.

But if we are going to have the L2 Provider fulfill both of these roles in kona-client, do you think it makes sense to refactor FPVMPrecompileOverride to just be generic over a single type that implements both traits?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this simplifies the ux and isn't clear who will need the split logic, I support doing this refactor in a follow-on pr. The FPVM Precompile Override can always be made even more abstract by encapsulating it's internal logic in a trait, so not too worried about increasing the abstractions here.

@refcell refcell self-requested a review July 23, 2024 13:56
@refcell refcell added this to the Phase 3: Kona Client & Host milestone Jul 23, 2024
Copy link
Collaborator

@refcell refcell left a comment

Choose a reason for hiding this comment

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

Generally looks good - just have a preference to keep serde as a feature flag with mutual exclusion.

@zobront zobront requested a review from refcell July 23, 2024 15:24
@refcell refcell enabled auto-merge July 23, 2024 15:34
@refcell refcell added this pull request to the merge queue Jul 23, 2024
Merged via the queue into op-rs:main with commit 22585bc Jul 23, 2024
19 checks passed
@github-actions github-actions bot mentioned this pull request Jul 23, 2024
@github-actions github-actions bot mentioned this pull request Aug 16, 2024
@github-actions github-actions bot mentioned this pull request Sep 2, 2024
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