linera-protocol icon indicating copy to clipboard operation
linera-protocol copied to clipboard

`linera_views::Context`: extract `KeyValueStore` functions

Open Twey opened this issue 7 months ago • 2 comments

Motivation

The Context trait currently duplicates the ReadableKeyValue and WritableKeyValue traits.

Proposal

Instead, provide direct access to the store so there's no need to pass through these calls.

While we're at it, factor BaseKey out into a separate type for the same reason.

Additionally, I noticed we've maintained the Local/Send split for our traits, a distinction that we chose to abandon in the codebase in favour of conditional compilation, so I merged those. This led to some knock-on effects:

  • we get to remove async-trait in a lot of places, replacing its trait transformation functionality with trait-variant where necessary — this simplifies the implementation of the traits, since implementers don't need to re-iterate the Sendness of the trait
  • since we can now no longer implement things for *Store without implementing them for Local*Store, we need to be more careful about the propagation of Send/Sync traits. Notably this required making GraphQL support conditional, which could even be extended to a feature flag in the future (just by modifying Cargo.toml and build.rs, since I made use of the with_* alias pattern).

Test Plan

CI.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Links

Twey avatar Apr 29 '25 18:04 Twey

You're right, getting tests passing resulted in a few more changes than originally intended :sweat_smile: I'll update the description. (The Cargo.toml changes were I think a rebase artefact — oops!)

Twey avatar May 02 '25 16:05 Twey

You're right, getting tests passing resulted in a few more changes than originally intended 😅 I'll update the description. (The Cargo.toml changes were I think a rebase artefact — oops!)

To be clear, removing unused crates is an unconditional positive. I just would like it to be documented.

MathieuDutSik avatar May 02 '25 17:05 MathieuDutSik

To be clear, removing unused crates is an unconditional positive. I just would like it to be documented.

Done! I think this may have happened when I ran cargo machete on the workspace by accident, but I guess since CI is happy they weren't necessary.

Looks great! But why are so many snap files getting deleted? Were these unused all along or did some tests get disabled due to conditional compilation?

They were unused all along. I realized by accident when in order to reset the snapshots I deleted them and then reran the tests. The only feature flag that affects these snapshots is metrics. When we change the snapshot name in the code (which can happen by accident, since it includes function name and type information) we currently have no means to automatically remove or highlight for removal the snapshots under the old name.

Twey avatar May 06 '25 12:05 Twey