Generalize client storage from the filesystem #2194

Twey · 2024-06-26T17:58:40Z

Motivation

The browser doesn't have access to a filesystem. In order to store and load wallet configuration, we must use a different backend, such as localStorage. Currently, though, the linera-service crate assumes filesystem access, and stores configuration as JSON on the filesystem.

Proposal

Generalize the WalletState struct with a new trait that represents any data that can be persisted. Move the previous behaviour into an implementation of that trait specialized to filesystem storage.

Test Plan

CI.

Release Plan

None required.

Links

reviewer checklist

…le::new`

ma2bd

LGTM. Letting @jvff or @afck accept

linera-service/src/persistent/file.rs

ma2bd · 2024-07-01T21:06:23Z

linera-service/src/persistent/file.rs

+    }
+}
+
+pub struct File<T> {


nit: The name File is a bit counter-intuitive: afaict this is a value persisted on file. Maybe OnFile<T> is a step in the right direction?

Another approach (which was highly debated before but perhaps ok here) is to use the module name for the distinction between different implementations. Something like persistence::file_system::Value implementing the trait persistence::(Persistent)Value.

This is a wrapper over the type (as indicated by the Deref impl), so I read the name persistent::File<Wallet> as ‘a persistent file containing a wallet’. The grammar of this name is hurt a bit with the rename to persistence, as well as file_system (a persistence::file_system::Value<Wallet> is not a persistence file system value containing a wallet). I agree that File should never be used unqualified!

So if you swap the storage medium, it will be called persistent::Web<Wallet> or persistent::Network<Wallet> ?

Something like persistent::LocalStorageEntry<Wallet> or persistent::S3Bucket<Wallet>, yes (the name refers to the entry that stores the value, not the medium the entry lives on).

ma2bd · 2024-07-01T21:09:03Z

linera-service/src/persistent/mod.rs

+
+pub use file::File;
+
+pub trait Persistent: Deref {


We could use some short (oneline) comment on public APIs.

nit: perhaps PersistentValue or Value (I don't mean to open a big debate though :))

Agree on the docs, good catch :) Though technically nothing in linera-service is ‘public’ as it can't be imported as a library, that will change in a little bit when this goes into linera-client :)

On the name — there's no clear consensus on this in Rust, but I prefer adjectives for trait names rather than nouns (as it avoids name clash with implementers, especially generic implementers). The competing convention is verbs, so this could be named Persist; happy to switch to that if we think it's better. If we want a noun then it should be agentive/patientive, i.e. Persistable, but suffixes like -able are specifically recommended against by the guidelines. I think Value is an essentially meaningless type name — anything nameable by a type is a value, so including the word ‘value’ in a type or trait name never adds information.

In fact I think I'm convinced that Persist is more in line with Rust (std::io::{Read,Write}, et cetera) — I'll rewrite to that unless you have objections.

I've added the docs and renamed Persistent to Persist.

This is more in line both with Rust standard library traits, which tend to be transitive verbs, and also with the Rust trait naming conventions RFC rust-lang/rfcs#344, which states: > Prefer (transitive) verbs, nouns, and then adjectives; avoid > grammatical suffixes (like able).

… handling, do not hide the outer error

afck

I love it! Not just more general, but also cleaner! 👍

afck · 2024-07-02T13:24:03Z

linera-service/src/proxy.rs

-        let genesis_config = GenesisConfig::read(&self.genesis_config_path)
-            .expect("Fail to read initial chain config");
+        let genesis_config: GenesisConfig =
+            util::read_json(&self.genesis_config_path).expect("Fail to read initial chain config");


The recommended style would be:

Suggested change

util::read_json(&self.genesis_config_path).expect("Fail to read initial chain config");

util::read_json(&self.genesis_config_path).expect("should read initial chain config");

(Also, I think @ma2bd prefers explicit generic arguments over type annotations, i.e. read_json::<GenesisConfig> over genesis_config: GenesisConfig?)

Hmm, I think that's the letter of the recommendation but not the spirit of it 😅 The message in expect is supposed to convey the invariant that we expect to lead to the Result being Ok. Probably this shouldn't be expect at all, but return the error up for anyhow.

On the turbofish — I don't see any official advice, but most Rustaceans (including the book) seem to prefer annotating the variable. It's less noisy and provides better (i.e. more localized) feedback when the type and value don't match. The turbofish is variously described as ‘a hack’ or ‘a wart’ that is unfortunately needed to keep the ability to provide generic arguments in some corner cases, but acceptable only because of how rarely it's encountered.

We have a lot of stuff to do so I think we can agree on some tolerance on this particular point for now. In my view, the problem with type annotations in the let is that they tend to stick around even after the reason for adding them has disappeared.

Oh yes, I was blocked on the tests flaking out, not this discussion :) It was clearly marked as a nit. IMO if it's not obvious whether a type annotation is needed or not, then it's nice to have it there for clarity anyway. But the same can be said of the turbofish — quite remote changes in the function, or even the signatures of other items that the function depends on, can obviate a turbofish without anybody noticing.

linera-service/src/persistent/mod.rs

linera-service/src/persistent/file.rs

linera-service/src/linera/main.rs

Co-authored-by: Andreas Fackler <[email protected]> Signed-off-by: James Kay <[email protected]>

…cumentation

Twey force-pushed the 06-26-Persistent_trait branch from 6905cc6 to e074913 Compare June 26, 2024 17:59

Twey changed the title ~~Persistent trait~~ Generalize client storage from the filesystem Jun 26, 2024

Twey force-pushed the 06-26-Persistent_trait branch 2 times, most recently from 0bb2f91 to ce83b06 Compare July 1, 2024 12:59

Twey marked this pull request as ready for review July 1, 2024 15:42

Twey added 4 commits July 1, 2024 17:12

linera-core: add Persistent trait

9dc6be5

linera-service: use new Persistent trait

2e9a581

linera-service: don't forget to create the file for `persistent::Fi…

35e06bd

…le::new`

linera-service: refresh PRNG seed more to align with tests

28df385

Twey force-pushed the 06-26-Persistent_trait branch from af493f9 to 28df385 Compare July 1, 2024 18:02

ma2bd requested review from ma2bd, afck and jvff July 1, 2024 20:20

ma2bd reviewed Jul 1, 2024

View reviewed changes

Twey added 3 commits July 2, 2024 11:15

linera-service: document Persist trait

d74441f

linera-service::persistent::File: when an error occurs during error…

665d251

… handling, do not hide the outer error

Twey requested a review from ma2bd July 2, 2024 13:07

afck approved these changes Jul 2, 2024

View reviewed changes

Twey and others added 4 commits July 2, 2024 16:49

linera_service::persistent: single-space documentation

e0c8034

Co-authored-by: Andreas Fackler <[email protected]> Signed-off-by: James Kay <[email protected]>

linera-service: mention bug linera-io#2053

a3928db

linera_service::proxy: replace expect with ?

7dfa45e

linera_service::persistent: use the indicative mood for function do…

d215310

…cumentation

Twey merged commit e27a55c into linera-io:main Jul 3, 2024
4 checks passed

This was referenced Jul 4, 2024

linera_core::client refactor #2083

Closed

linera-client: generalize over Persist<Target = Wallet> implementation #2224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize client storage from the filesystem #2194

Generalize client storage from the filesystem #2194

Twey commented Jun 26, 2024

ma2bd left a comment

ma2bd Jul 1, 2024 •

edited

Loading

ma2bd Jul 1, 2024

Twey Jul 2, 2024 •

edited

Loading

ma2bd Jul 2, 2024

Twey Jul 2, 2024

ma2bd Jul 1, 2024

ma2bd Jul 1, 2024 •

edited

Loading

Twey Jul 2, 2024 •

edited

Loading

Twey Jul 2, 2024 •

edited

Loading

Twey Jul 2, 2024

afck left a comment

afck Jul 2, 2024

afck Jul 2, 2024

Twey Jul 2, 2024

Twey Jul 2, 2024 •

edited

Loading

ma2bd Jul 2, 2024

Twey Jul 3, 2024

	util::read_json(&self.genesis_config_path).expect("Fail to read initial chain config");
	util::read_json(&self.genesis_config_path).expect("should read initial chain config");

Generalize client storage from the filesystem #2194

Generalize client storage from the filesystem #2194

Conversation

Twey commented Jun 26, 2024

Motivation

Proposal

Test Plan

Release Plan

Links

ma2bd left a comment

Choose a reason for hiding this comment

ma2bd Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twey Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2bd Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Twey Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Twey Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twey Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2bd Jul 1, 2024 •

edited

Loading

Twey Jul 2, 2024 •

edited

Loading

ma2bd Jul 1, 2024 •

edited

Loading

Twey Jul 2, 2024 •

edited

Loading

Twey Jul 2, 2024 •

edited

Loading

Twey Jul 2, 2024 •

edited

Loading