chore: separate fedimint config info from gateway info endpoint #3880

mayrf · 2023-12-08T16:23:06Z

Fixes #3815. I am not sure it's completely done and would appreciate feedback.

codecov · 2023-12-08T16:30:32Z

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (b891544) 57.09% compared to head (dc7c1f9) 57.04%.
Report is 65 commits behind head on master.

Files	Patch %	Lines
gateway/ln-gateway/src/lib.rs	29.62%	19 Missing ⚠️
gateway/ln-gateway/src/rpc/rpc_server.rs	28.57%	5 Missing ⚠️
gateway/cli/src/main.rs	0.00%	4 Missing ⚠️
gateway/ln-gateway/src/rpc/rpc_client.rs	33.33%	4 Missing ⚠️
gateway/ln-gateway/src/rpc/mod.rs	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3880      +/-   ##
==========================================
- Coverage   57.09%   57.04%   -0.06%     
==========================================
  Files         193      193              
  Lines       42649    43146     +497     
==========================================
+ Hits        24351    24613     +262     
- Misses      18298    18533     +235

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

elsirion

Looks good, but I think we can simplify it a bit more.

elsirion · 2023-12-08T17:20:52Z

gateway/cli/src/main.rs

@@ -30,6 +30,8 @@ pub enum Commands {
    VersionHash,
    /// Display high-level information about the Gateway
    Info,
+    /// Display config information about the Gateways federation
+    Config,


Imo this should be a per-federation command, i.e. takes the federation id as argument.

I wonder which is better. This or --federation-id acting as a filter, and lack of it meaning "all".

Why do we need the "all" version of the command?

I don't know. If you're in one federation only you can get the config without specifying the id. Or if you're writing a script you could loop over all in one go (one | jq ...). Not very strong reasons, but also doesn't change anything for someone that just wants one config.

elsirion · 2023-12-08T17:21:41Z

gateway/ln-gateway/src/lib.rs

+        client: &ClientArc,
+        federation_id: FederationId,
+    ) -> FederationConfigInfo {
+        let balance_msat = client.get_balance().await;


Do we need the balance here? We already have an API endpoint for that.

Balance doesn't seem like part of a "config".

elsirion · 2023-12-08T17:22:33Z

gateway/ln-gateway/src/lib.rs

+        FederationConfigInfo {
+            federation_id,
+            balance_msat,
+            config,
+        }


If we allow config fetching per federation only then we don't need to include the federation id in the response.

If we remove "balance", ... should it be just a map? id -> config ?

Why not just a config? Just call the command once for each federation you care about (likely only one at a time anyway)

I guess it's a follow up to do we allow listing all or not. If we do , will be easier to parse/filter, if not it's kind of needless.

dpc · 2023-12-08T17:57:56Z

scripts/dev/mprocs/run.sh

@@ -8,4 +8,4 @@ ensure_in_dev_shell
 build_workspace
 add_target_dir_to_path

-devimint --link-test-dir ./target/devimint "$@" dev-fed --exec bash -c 'mprocs -c misc/mprocs.yaml 2>$FM_LOGS_DIR/devimint-outer.log'
+devimint --link-test-dir ./target/devimint "$@" dev-fed --exec bash -c 'mprocs -c misc/mprocs.yaml 2>$FM_LOGS_DIR/devimint-outer.high-levellog'


question: ?

mayrf · 2023-12-18T00:07:38Z

Sorry for the delay. I went for dpcs suggestion and implemented an --federation-id option, returning either the config of the federation specified or those of all federations, in case no federation-id is specified.

elsirion · 2023-12-18T11:49:22Z

gateway/ln-gateway/src/rpc/rpc_client.rs

@@ -94,7 +99,7 @@ impl GatewayRpcClient {
            .base_url
            .join("/set_configuration")
            .expect("invalid base url");
-        self.call(Method::POST, url, Some(payload)).await
+        self.call_post(url, payload).await


Do we want to have composite calls in fedimint-cli? Can be fixed in another PR.

okjodom · 2023-12-18T12:34:55Z

gateway/ln-gateway/src/lib.rs

+            let mut federations = BTreeMap::new();
+            if let Some(federation_id) = federation_id {
+                let client = self.select_client(federation_id).await?;
+                // federations.insert(federation_id, client.get_config().clone());


nit: let's clean up commented code

okjodom · 2023-12-18T12:36:47Z

gateway/ln-gateway/src/rpc/mod.rs

+#[derive(Debug, Serialize, Deserialize, PartialEq)]
+pub struct GatewayFedConfig {
+    pub federations: BTreeMap<FederationId, JsonClientConfig>,
+    // pub federations: BTreeMap<FederationId, ClientConfig>,


nit: let's clean up commented code

okjodom

left nit comments, yet this looks and works great!

tACK 1bc5bfd

mayrf requested review from a team as code owners December 8, 2023 16:23

elsirion reviewed Dec 8, 2023

View reviewed changes

dpc reviewed Dec 8, 2023

View reviewed changes

mayrf force-pushed the split-gateway-info-endpoint branch 4 times, most recently from f3126d7 to 1bc5bfd Compare December 18, 2023 00:06

mayrf requested review from elsirion and dpc December 18, 2023 11:16

elsirion previously approved these changes Dec 18, 2023

View reviewed changes

okjodom reviewed Dec 18, 2023

View reviewed changes

okjodom previously approved these changes Dec 18, 2023

View reviewed changes

chore: separate fedimint config info from gateway info endpoint

dc7c1f9

mayrf dismissed stale reviews from okjodom and elsirion via dc7c1f9 December 18, 2023 15:48

mayrf force-pushed the split-gateway-info-endpoint branch from 1bc5bfd to dc7c1f9 Compare December 18, 2023 15:48

mayrf mentioned this pull request Dec 18, 2023

Fix gateway-cli error handling and auto-completion #3966

Open

okjodom approved these changes Dec 18, 2023

View reviewed changes

elsirion approved these changes Dec 20, 2023

View reviewed changes

elsirion added this pull request to the merge queue Dec 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2023

elsirion added this pull request to the merge queue Dec 20, 2023

Merged via the queue into fedimint:master with commit 251b16e Dec 20, 2023
20 checks passed

elsirion mentioned this pull request Jan 15, 2024

fix(gateway): re-introduce post info API endpoint #4037

Closed

elsirion mentioned this pull request Jan 16, 2024

Add backwards-compatible test matrix #4010

Merged

bradleystachurski mentioned this pull request Jan 25, 2024

fix: make gateway api backwards-compatible #4136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: separate fedimint config info from gateway info endpoint #3880

chore: separate fedimint config info from gateway info endpoint #3880

mayrf commented Dec 8, 2023 •

edited

codecov bot commented Dec 8, 2023 •

edited

elsirion left a comment

elsirion Dec 8, 2023

dpc Dec 8, 2023

elsirion Dec 8, 2023

dpc Dec 11, 2023

elsirion Dec 8, 2023

dpc Dec 8, 2023

elsirion Dec 8, 2023

dpc Dec 8, 2023

elsirion Dec 8, 2023

dpc Dec 11, 2023

dpc Dec 8, 2023

mayrf commented Dec 18, 2023

elsirion Dec 18, 2023

okjodom Dec 18, 2023 •

edited

okjodom Dec 18, 2023

okjodom left a comment

chore: separate fedimint config info from gateway info endpoint #3880

chore: separate fedimint config info from gateway info endpoint #3880

Conversation

mayrf commented Dec 8, 2023 • edited

codecov bot commented Dec 8, 2023 • edited

Codecov Report

elsirion 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayrf commented Dec 18, 2023

Choose a reason for hiding this comment

okjodom Dec 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okjodom left a comment

Choose a reason for hiding this comment

mayrf commented Dec 8, 2023 •

edited

codecov bot commented Dec 8, 2023 •

edited

okjodom Dec 18, 2023 •

edited