feat: dev commands to decode and encode notes and invite_code to and from json #4473

Kodylow · 2024-03-06T04:51:01Z

replaces #4449

Adds dev codes for fedimint-cli to encode/decode to/from json for notes and invite_code

Would it be out of scope to add cashu encoding here? It'd just rename the keys and put the amount in each of the notes then spit out the json

elsirion · 2024-03-06T07:13:56Z

fedimint-cli/src/lib.rs

+                    federation_id_prefix: notes.federation_id_prefix().to_string(),
+                    notes: notes.notes().clone(),


I actually liked the previous version of your JSON encoding code which was able to display all OOBNoteData variants. Now we are missing invite codes when provided.

updated to use it on OOB notes through .notes_json

elsirion · 2024-03-06T07:21:21Z

fedimint-cli/src/lib.rs

+        #[clap(long = "federation_id_prefix")]
+        federation_id_prefix: String,
+        #[clap(long = "notes")]
+        notes: String,


~~Shouldn't this be a Vec<_>? Ideally a Vec<SpendableNote>, but due to clap-rs/clap#1704 likely rather a Vec<String>.~~ Maybe supplying the whole vec as JSON is actually more convenient.

I originally had it as a vec but it's super tedious to input that way, doing it as Json string you can just wrap the JSON output from the dev decode command with single quotes like '{"notes": stuff}'

elsirion · 2024-03-06T07:22:10Z

fedimint-cli/src/lib.rs

+        #[clap(long = "url")]
+        url: SafeUrl,


I think if the field name is the same as the long argument name you can just annotate it as #[clap(long)]

elsirion · 2024-03-07T07:29:04Z

Breaks backwards compat tests, I had to deal with that in #4415, I bet you also have to do some version switching for the command to use for decoding since you changed the CLI.

dpc · 2024-03-11T02:53:14Z

devimint/src/tests.rs

@@ -346,6 +346,8 @@ pub async fn latency_tests(dev_fed: DevFed, r#type: LatencyTest) -> Result<()> {
 }

 pub async fn cli_tests(dev_fed: DevFed) -> Result<()> {
+    let fedimint_cli_version = crate::util::FedimintCli::version_or_default().await;
+    let version_req = VersionReq::parse(">=0.3.0-alpha")?;


If I see correctly, this variable is used only once, and having it here just makes it far from where it's used.

I can move it down, was thinking that eventually all the tests will require version matching so define up top but nbd

Fair enough. 👍

But... in that case it might need a longer name.

dpc · 2024-03-11T02:55:48Z

modules/fedimint-mint-client/src/lib.rs

@@ -154,6 +155,44 @@ impl OOBNotes {
            .expect("Invariant violated: OOBNotes does not contain any notes")
    }

+    pub fn notes_json(&self) -> Result<serde_json::Value, serde_json::Error> {
+        let mut map = serde_json::Map::new();


Nit: map is a meh name for a variable - it doesn't describe the meaning of it in it.

dpc · 2024-03-11T19:35:27Z

I'd just squash it all at this point, no biggy.

dpc

LGTM, but really could use a squash.

dpc · 2024-03-15T06:09:33Z

fix: backwards compatibility tests

fix: fix

fix: test

fix: hold

fix: tests fix

fix: encoding ecash

fix: decode on OOBNotesData

feat: encode dev command for notes and invite-code

feat: decode notes to json

not a good commit message :P

m1sterc001guy · 2024-03-15T17:25:41Z

modules/fedimint-mint-client/src/lib.rs

+        for notes in self.0.iter() {
+            match notes {
+                OOBNotesData::Notes(notes) => {
+                    let notes_json = serde_json::to_value(notes).expect("serialization can't fail");


Nit: the other to_value return Result rather than expect, you could do the same here

m1sterc001guy · 2024-03-15T17:28:05Z

devimint/src/tests.rs

+    .await?;
+
+    anyhow::ensure!(
+        encode_invite_output["invite_code"].as_str().unwrap() == invite,


Nit: use expect instead of unwrap

dpc

But linters not happy

elsirion · 2024-03-19T10:24:42Z

modules/fedimint-mint-client/src/lib.rs

+                    let notes_json = serde_json::to_value(notes)?;
+                    notes_map.insert("notes".to_string(), notes_json);


We should probably assert that insert returns None to avoid confusing ourselves should we ever allow multiple instances of some data type.

Kodylow requested a review from a team as a code owner March 6, 2024 04:51

Kodylow mentioned this pull request Mar 6, 2024

feat: parse-json for oobNotes and cli-command #4449

Closed

elsirion reviewed Mar 6, 2024

View reviewed changes

Kodylow force-pushed the kl/dev-decode-encode branch from ef0ae5b to 0ac21f9 Compare March 6, 2024 20:50

Kodylow force-pushed the kl/dev-decode-encode branch 4 times, most recently from 8e9eb29 to 9b31257 Compare March 10, 2024 20:13

dpc reviewed Mar 11, 2024

View reviewed changes

dpc previously approved these changes Mar 11, 2024

View reviewed changes

Kodylow dismissed dpc’s stale review via eabc220 March 11, 2024 16:11

Kodylow force-pushed the kl/dev-decode-encode branch from eabc220 to 504638f Compare March 11, 2024 16:11

dpc previously approved these changes Mar 11, 2024

View reviewed changes

Kodylow force-pushed the kl/dev-decode-encode branch 2 times, most recently from 0b6732f to f48d917 Compare March 14, 2024 18:36

Kodylow dismissed dpc’s stale review via b446279 March 15, 2024 03:33

dpc reviewed Mar 15, 2024

View reviewed changes

Kodylow force-pushed the kl/dev-decode-encode branch from b446279 to 776ff93 Compare March 15, 2024 16:14

m1sterc001guy reviewed Mar 15, 2024

View reviewed changes

m1sterc001guy previously approved these changes Mar 15, 2024

View reviewed changes

Kodylow dismissed m1sterc001guy’s stale review via 4b61bd9 March 15, 2024 19:24

Kodylow force-pushed the kl/dev-decode-encode branch 2 times, most recently from 4b61bd9 to 49addcc Compare March 15, 2024 19:30

m1sterc001guy previously approved these changes Mar 15, 2024

View reviewed changes

dpc previously approved these changes Mar 15, 2024

View reviewed changes

Kodylow dismissed stale reviews from dpc and m1sterc001guy via efc6b0c March 16, 2024 04:04

Kodylow force-pushed the kl/dev-decode-encode branch 3 times, most recently from 51cfec1 to 632281d Compare March 18, 2024 17:25

dpc previously approved these changes Mar 18, 2024

View reviewed changes

Kodylow dismissed dpc’s stale review via 5d01f70 March 18, 2024 20:24

Kodylow force-pushed the kl/dev-decode-encode branch from 632281d to 5d01f70 Compare March 18, 2024 20:24

Kodylow added 2 commits March 18, 2024 15:28

feat: dev decode/encode notes/invites to json

4926eab

fix: rebase fix .map_err_cli_msg

c7d12ab

Kodylow force-pushed the kl/dev-decode-encode branch from 5d01f70 to c7d12ab Compare March 18, 2024 22:35

m1sterc001guy approved these changes Mar 19, 2024

View reviewed changes

dpc approved these changes Mar 19, 2024

View reviewed changes

dpc added this pull request to the merge queue Mar 19, 2024

Merged via the queue into fedimint:master with commit 2befd3c Mar 19, 2024
20 checks passed

elsirion reviewed Mar 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dev commands to decode and encode notes and invite_code to and from json #4473

feat: dev commands to decode and encode notes and invite_code to and from json #4473

Kodylow commented Mar 6, 2024

elsirion Mar 6, 2024

Kodylow Mar 6, 2024

elsirion Mar 6, 2024

Kodylow Mar 6, 2024

elsirion Mar 6, 2024

Kodylow Mar 6, 2024

elsirion commented Mar 7, 2024

dpc Mar 11, 2024

Kodylow Mar 11, 2024

dpc Mar 11, 2024

dpc Mar 11, 2024

dpc commented Mar 11, 2024

dpc left a comment

dpc commented Mar 15, 2024

m1sterc001guy Mar 15, 2024

m1sterc001guy Mar 15, 2024

dpc left a comment

elsirion Mar 19, 2024

		federation_id_prefix: notes.federation_id_prefix().to_string(),
		notes: notes.notes().clone(),

		let notes_json = serde_json::to_value(notes)?;
		notes_map.insert("notes".to_string(), notes_json);

feat: dev commands to decode and encode notes and invite_code to and from json #4473

feat: dev commands to decode and encode notes and invite_code to and from json #4473

Conversation

Kodylow commented Mar 6, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsirion commented Mar 7, 2024

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

dpc commented Mar 11, 2024

dpc left a comment

Choose a reason for hiding this comment

dpc commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment