fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 209 forks source link

Implement filtering and details on gatewayd info API #1725

Closed okjodom closed 4 months ago

okjodom commented 1 year ago

Today, gateway-cli info returns high level information on all connected federations. Consider supporting filters on gatway info api, so we can use cli or api call to get information about a specific federation served by the gateway.

Proposal:

okjodom commented 1 year ago

Go for it :)

okjodom commented 1 year ago

However, I recommend you wait for #1718 to land so you have a clean starting point requiring you to implement changes in one place only

justinmoon commented 1 year ago

You could also just branch off #1718 and then make a PR based on that. Should be compatible once #1718 gets merged.

oleonardolima commented 1 year ago

@okjodom the previous assignee has been removed 🤔 Is this up for takers again? I can give it a try :)

okjodom commented 1 year ago

Go for it! There should be no blockers for tacking this issue right away.

Afaik (assumption), @kushalnl7 dropped their interest in this issue by removing their own assignment. Go ahead and claim this issue, @oleonardolima.

And yes, there's tons of other good first issues in this space, so we won't run out of room to contribute :)

oleonardolima commented 1 year ago

@okjodom Great! I'll start working on it then 🚀

oleonardolima commented 1 year ago

@okjodom I started digging into this one and I would like to check if the path that I have in mind is what's expected.

So, the idea besides adding a new option on the cli command, would be to have another route on the rpc_server, right ?

Also, add a new type with richer federation info on the ln-gateway module which would be used by this new route, right ?

Or the idea is to not add a new route at all, and use the current paths and combine them only somehow at cli client level ? (btw, I don't think that this would be a good approach 🤔, but wanted to check with you)

okjodom commented 1 year ago

he idea is to not add a new route at all, and use the current paths and combine them only somehow at cli client level ?

This is the idea. Instead of adding more API endpoints to the gateway webserver, we should have federation-id> as an optional query parameter on the existing /info endpoint

btw, I don't think that this would be a good approach thinking

why, what are your thoughts?

oleonardolima commented 1 year ago

Cool! I'll follow this path then

why, what are your thoughts?

Ah, because by the other approach, I was thinking it could be multiple calls to existing routes, instead of passing a query parameter, that would need to handle and combine the data on cli client and I guess wouldn't be that great 😅

oleonardolima commented 1 year ago

@okjodom I got myself with two questions related to the tests:

I was wondering at which place would be appropriate to add some tests related to the change 🤔, am I missing something ?

okjodom commented 1 year ago

We have no specific tests for the gateway-cli. cli-test.sh is meant to test the fedimint-cli, and not gateway-cli. However, you might find an instance where gateway-cli is applied in the course of testing fedimint cli.

Opinion: I don't think we wan't a shell test for gateway-cli.

You probably noticed that gateway-cli uses an instance of this RpcClient to interact with Gatewayd API. Now a similar instance of RpcClient is used in existing gatewayd athentication_tests thus there's already some coverage on the APIs from that angle. I'm proposing an extensive gatewayd test suite that would excersise RpcClient API even further.

Gap: Given RpcClient api surface is naturally tested by it's application in gatewayd tests, If we needed tests for gatewayd-cli, we'd scope them to testing of the CLI option parsing and error handling only.

Here is another related WIP, simplifying the gateway test structure

justinmoon commented 1 year ago

the specific tests for gateway-cli are those in cli-test.sh ?

In the next week the way we test the CLIs is going to change a lot. We'll replace the cli-test.sh with some tests in Rust that call out the the shell. At that point would be a good time to write some tests for the gateway-cli. @Maan2003 working on this migration here FYI -> https://github.com/fedimint/fedimint/pull/1910.

Most of the current test coverage for the gateway is in integration tests and cli-tests.sh which does incoming and outgoing payments via gateway.

justinmoon commented 1 year ago

A potentially good first step in this direction would be to allow naming gateways. Then you could do commands with gateway-cli info <name> where name is something you can remember. Pasting federation IDs isn't fun!

oleonardolima commented 1 year ago

We have no specific tests for the gateway-cli. cli-test.sh is meant to test the fedimint-cli, and not gateway-cli. However, you might find an instance where gateway-cli is applied in the course of testing fedimint cli.

Oh, I misunderstood it, thanks for the explanation. Yes, that was it I saw the usage of gateway-cli and inferred that 😅

I'm https://github.com/fedimint/fedimint/pull/1702 that would excersise RpcClient API even further.

Good! I'll take a look at it.

--

In the next week the way we test the CLIs is going to change a lot. We'll replace the cli-test.sh with some tests in Rust that call out the the shell ...

Thanks, I'll check it and I'll give it a try on a test the change only for the Gatewayd API for now.

A potentially good first step in this direction would be to allow naming gateways...

Good point, this seems really interesting 🤔

oleonardolima commented 1 year ago

@okjodom I'm not yet well aware of what info would be a must, should I just focus on the balance and summary of transactions for now?

BTW: I'm hoping to open the PR later today (tonight) 😁

okjodom commented 1 year ago

@okjodom I'm not yet well aware of what info would be a must, should I just focus on the balance and summary of transactions for now?

BTW: I'm hoping to open the PR later today (tonight) grin

ATM I'm only keen on seeing the filtering ability on the API.

gateway-cli info continues to return high level info about all connected federations. Info should include:

  • list of federations.
  • each federation in list should show federation id gateway-cli info should return detailed info about the federation identified. Info should include:
  • federation id
  • balance held in federation

We will then build out these API further in future. For example, when we build transaction history and accounting within a federation, we might want to show that in the detailed gateway-cli info <federation-id>

elsirion commented 4 months ago

Dup of the already fixed #3815.