RPC server for API Interface

[yamabiiko]

Opening this PR so it can start getting reviewed and possibly get some help. As discussed in #1185, this is quite a substantial rework which adds the possibility of running an RPC server and unifies the RPC methods and CLI commands under an internal API.

To be done before we can get this merged:

• [x] Tests: fixing and expanding tests in swap/tests/rpc.rs: right now the test runs the RPC server but the functionality cannot be tested as the Bitcoin wallet does not have any balance and the swap database is empty (so right now it's using some placeholder BTC/XMR addresses and swap ids and it will always fail).
• [x] Tests: write more tests (containers?)
• [x] Reevaluate the placing of the new files (request.rs, api.rs, rpc.rs) within the project file strucuture
• [ ] Organize commits by squashing some, changing commit messages to be more descriptive to make reviewing this easier
• [ ] Get the entire code reviewed by one other person (@delta1)

If someone could help with writing proper tests (or pointing out at some examples in the code to how swaps and transactions can be simulated/tested) it would be appreciated.

[binarybaron]

@delta1 @yamabiiko Please give as much feedback and scrutiny as possible. The API is really important and we need to make sure that it's stable enough to be used by a lot of GUIs etc. in the future. I'll also give feedback in the next days and try to be as thorough as possible.

[delta1]

Will review when I can in the next few days. @yamabiiko is there a reason for duplicated code instead of importing from other modules?

[yamabiiko]

Will review when I can in the next few days. @yamabiiko is there a reason for duplicated code instead of importing from other modules?

Could you point out the specific examples?

[yamabiiko]

Another thing that still needs to be addressed is printing the tables/vectors with the History and ListSellers commands (just like before with the CLI)

[binarybaron]

True + we need to be able to associate log messages to individual rpc requests/commands/swaps

[yamabiiko]

Could this be reviewed and tested again? Specifically regarding CLI/Json output. The only thing I see that's missing right now is some tests for checking if concurrent swaps are correctly handled.

[binarybaron]

I'll take a look. Thanks!

[binarybaron]

Could this be reviewed and tested again? Specifically regarding CLI/Json output. The only thing I see that's missing right now is some tests for checking if concurrent swaps are correctly handled.

Are concurrent swaps handled at all?

[binarybaron]

Here is a list of things that we still need to get done before we can get this merged. I'll also try to help where I can and make some commits. I'll update this list when needed.

• [X] Currently we save the logs of each swap to an individual log file (swap-SWAPID.log). This allows other programs (e.g the GUI) to parse information from those log files about the state of the certain swaps. We need to continue this behaviour. I think this can be achieved by using spans (discarded)
• [X] It'd be best if we could associate each log message that is printed to the corresponding API command that caused it. Again, I believe this can be done using spans
• [X] We need to be able to forcefully terminate a running swap without crashing the RPC server
• [X] We need to prevent the user from initiating multiple swaps concurrently
• [ ] We need to add an endpoint to retrieve information about the currently running swap
• [X] We should look into combining the Cmd and Params struct

[binarybaron]

Here is a list of things that we still need to get done before we can get this merged. I'll also try to help where I can and make some commits. I'll update this list when needed.

• [ ] Currently we save the logs of each swap to an individual log file (swap-SWAPID.log). This allows other programs (e.g the GUI) to parse information from those log files about the state of the certain swaps. We need to continue this behaviour. I think this can be achieved by using spans
• [X] It'd be best if we could associate each log message that is printed to the corresponding API command that caused it. Again, I believe this can be done using spans
• [ ] We need to be able to forcefully terminate a running swap without crashing the RPC server

I made a commit to fix the second bullet point. Appending each log message conditionally to a log file based on the swap id is not as simple.

I've begin working on the first point on the branch https://github.com/comit-network/xmr-btc-swap/tree/rpc-server-tracing-log-files. Its not as easy as you'd think and I'm struggling to format the log messages correctly to append them to the log file. If any of you two have some hints/ideas how to implement this, feel free to share @delta1 @yamabiiko

This is the relevant code: https://github.com/comit-network/xmr-btc-swap/blob/c32bb6f6248304a740f91d2298a7d367d7439628/swap/src/cli/tracing.rs#L59-L71

[yamabiiko]

Should the running swap be cancelled with a separate method? Or should we do it when CancelAndRefund gets called?

[binarybaron]

Seperate method. CancelAndRefund just means that both the cancel and refund transactions are published.

[binarybaron]

Here is a list of things that we still need to get done before we can get this merged. I'll also try to help where I can and make some commits. I'll update this list when needed.

• [x] Currently we save the logs of each swap to an individual log file (swap-SWAPID.log). This allows other programs (e.g the GUI) to parse information from those log files about the state of the certain swaps. We need to continue this behaviour. I think this can be achieved by using spans (discarded)
• [x] It'd be best if we could associate each log message that is printed to the corresponding API command that caused it. Again, I believe this can be done using spans
• [x] We need to be able to forcefully terminate a running swap without crashing the RPC server
• [x] We need to prevent the user from initiating multiple swaps concurrently
• [ ] We need to add an endpoint to retrieve information about the currently running swap
• [x] We should look into combining the Cmd and Params struct

This PR is getting closer and closer to being finished. We're mostly missing two things:

• [ ] The tests are not complete and not working atm
• [ ] We want to make it possible to send push notifications to API callers about the currently running swap. Add a callback(?) to the context to allow the BuyXmr/Resume method to push JSON RPC notifications to the API caller to notify them about the status of the swap
• [ ] Reevaluate the placing of the new files (request.rs, api.rs, rpc.rs) within the project file strucuture

Due to the massive changes triggered by this PR and its sheer size, I'll make an special exception to the bounty system for this one. All reviewers, excluding me, will receive a payout of 1 XMR for conducting a thorough review of the code. I want to make sure that we minimise the chance of breaking anything major here and I think the risk is quite high due to the large scale modifications to the whole codebase.

In practice, this will probably only apply to you @delta1 unless someone else with sufficient knowledge of rust and this codebase come forth as well.

[yamabiiko]

Here is a list of things that we still need to get done before we can get this merged. I'll also try to help where I can and make some commits. I'll update this list when needed.

• [x] Currently we save the logs of each swap to an individual log file (swap-SWAPID.log). This allows other programs (e.g the GUI) to parse information from those log files about the state of the certain swaps. We need to continue this behaviour. I think this can be achieved by using spans (discarded)
• [x] It'd be best if we could associate each log message that is printed to the corresponding API command that caused it. Again, I believe this can be done using spans
• [x] We need to be able to forcefully terminate a running swap without crashing the RPC server
• [x] We need to prevent the user from initiating multiple swaps concurrently
• [ ] We need to add an endpoint to retrieve information about the currently running swap
• [x] We should look into combining the Cmd and Params struct

This PR is getting closer and closer to being finished. We're mostly missing two things:

• [ ] The tests are not complete and not working atm
• [ ] We want to make it possible to send push notifications to API callers about the currently running swap. Add a callback(?) to the context to allow the BuyXmr/Resume method to push JSON RPC notifications to the API caller to notify them about the status of the swap
• [ ] Reevaluate the placing of the new files (request.rs, api.rs, rpc.rs) within the project file strucuture

Due to the massive changes triggered by this PR and its sheer size, I'll make an special exception to the bounty system for this one. All reviewers, excluding me, will receive a payout of 1 XMR for conducting a thorough review of the code. I want to make sure that we minimise the chance of breaking anything major here and I think the risk is quite high due to the large scale modifications to the whole codebase.

In practice, this will probably only apply to you @delta1 unless someone else with sufficient knowledge of rust and this codebase come forth as well.

I am working on the second point, when that will be done I will take care of tests as well (though if someone else wants to they can help).

I will also review everything when we have completed this checklist, but as binarybaron mentioned an external reviewer would be really beneficial to the codebase quality and reliability.

[binarybaron]

I updated the PR description

[binarybaron]

@delta1 When you review this can you give a comment on the placing of the request.rs, api.rs, rpc.rs files within the project structure. I'm not too familiar with how this is typically done in rust projects.

[binarybaron]

@yamabiiko the code you pushed for moving initialization out of the spawned task looks good to me! Can you implement the same for BuyXmr as well?

[yamabiiko]

@yamabiiko the code you pushed for moving initialization out of the spawned task looks good to me! Can you implement the same for BuyXmr as well?

Yes, but I need to refactor the code to avoid borrowing the context before moving, I'll work on it

[binarybaron]

@yamabiiko the code you pushed for moving initialization out of the spawned task looks good to me! Can you implement the same for BuyXmr as well?

Yes, but I need to refactor the code to avoid borrowing the context before moving, I'll work on it

Yeah I had some issues with that too when I tried to implement this

[binarybaron]

We broke rustfmt... :(

Message: Child process exited with code 1: error[internal]: left behind trailing whitespace

[binarybaron]

This is ready for review @delta1

[delta1]

This is ready for review @delta1

thanks i should be able to review again next week

[binarybaron]

bors try

[delta1]

resolved conflicts, running CI

[delta1]

pushed clippy and minor fixes

will review further soon

[delta1]

I think the RPC test needs to be in CI and modified before this can be merged. Since the tests need to run in serial, it might as well be one test that just runs through the various calls.

[yamabiiko]

I think leaving the tests separated is fine to avoid waiting on one big slow test.

[delta1]

I think leaving the tests separated is fine to avoid waiting on one big slow test.

I disagree, especially since they don't work.

Conflicts need to be resolved to run CI and then will re-review @yamabiiko

[yamabiiko]

I disagree, especially since they don't work.

Could you elaborate more? Locally they work fine, and I don't see them fail in the CI. It seems the Monero wallet RPC failed but that should be unrelated.

[delta1]

@yamabiiko: sorry, I seemed to remember it failed last time I tried it.

Regardless, it's extremely inefficient since they must be run serially, and also doing the container setup and teardown for each test. It's already been running for 17 minutes (and I see it has failed now actually).

We're almost there with this PR, but these RPC tests must be reduced to as few cases as possible. All the cases that can be tested at the same point in a swap should be done in the same test.