Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider changing interface of internal send_request function #47

Open
thomaseizinger opened this issue Aug 20, 2021 · 2 comments
Open

Comments

@thomaseizinger
Copy link
Owner

One of the design goals of this client is to allow for easy generation of proxies by implementing the trait yourself and forwarding to an inner implementation.

At the moment, this doesn't work quite well because the generated send_request function doesn't do the parsing of the response and hence anyone overwriting that function cannot access stuff like the JsonRpcError.

thomaseizinger added a commit to comit-network/xmr-btc-swap that referenced this issue Aug 20, 2021
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
thomaseizinger added a commit to comit-network/xmr-btc-swap that referenced this issue Aug 26, 2021
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
thomaseizinger added a commit to comit-network/xmr-btc-swap that referenced this issue Aug 26, 2021
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
@gggto
Copy link

gggto commented Sep 27, 2021

Regarding this issue, shouldn't the SendRequest trait look something along these lines:

#[async_trait::async_trait]
pub trait SendRequest: 'static
{
    type Error: StdError;

    async fn send_request<P>(
        &self,
        endpoint: Url,
        body: String,
    ) -> Result<Response<P>, Error<Self::Error>>
    where
        P: DeserializeOwned;
}

Where the From requirement is gone, the resulting Err being a jsonrpc_client::Error. That coupled with a general impl for Error casting ergonomics:

impl<T> From<T> for crate::Error<T> {
    fn from(inner: T) -> Self {
        crate::Error::Client(inner)
    }
}

With this, we would:

  1. Be able to Implement SendRequest with errors outside the crate,
  2. Still be able to cast the inner error type to the jsonrpc_client error type,
  3. Be able to report parsing errors of the response json with the JsonRpc error variant.

Or, another take would be that maybe parsing the json response should be handled by the caller, replacing the Response<P> in the return type with maybe a String or something. In that case, we would do something like that:

#[async_trait::async_trait]
pub trait SendRequest: 'static
{
    type Error: StdError;

    async fn send_request(
        &self,
        endpoint: Url,
        body: String,
    ) -> Result<String, Self::Error>
}

While keeping the impl<T> From<T> for crate::Error<T>. But that second option is more subject to debate IMHO, not sure what is the right thing to do.

@thomaseizinger
Copy link
Owner Author

Feel free to give the first one a shot, I don't remember whether I just didn't try this or if there are any issues why it doesn't work!

Happy to discuss over a draft PR as well 😊

abraham-nixon added a commit to abraham-nixon/xmr-btc-swap that referenced this issue Feb 15, 2022
Previously, we relied on the wallet in the `monero-wallet-rpc` daemon
to be loaded as we do on startup. As a consequence of this expectation,
restarting `monero-wallet-rpc` to fix bugs like #652 resulted in the
ASB no longer operating correctly.

To fix this, we now load the wallet on-demand in case the daemon responds
with the error code -13.

Ideally, we would implement this behaviour generically using the proxy
pattern on the `MoneroWalletRpc` trait. Unfortunately, when attempting
to do so we uncover a limitation in the design of `jsonrpc_client`.
This limitation is tracked in thomaseizinger/rust-jsonrpc-client#47.
Once fixed, we can implement this logic in a more robust way that is not
tied to the `check_tx_key` RPC call but applies to any RPC call automatically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants