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

RPC authentication #86

Closed
wants to merge 9 commits into from
Closed

Conversation

refring
Copy link
Contributor

@refring refring commented Oct 17, 2022

This PR implements #43 for both monerod rpc and wallet rpc

Docker containers with --rpc-login enabled have been added for both monerod and monero-wallet-rpc, tests are included as well.

Currently I added a new constructor RpcClient::with_authentication() but seeing that both #77 and #81 are using the current RpcClient::new() in a backwards incompatible way I also made a branch with a new RpcClient builder called RpcClientBuilder which would allow for backwards compatibility and enable all three new features to be used together.

This is can be seen at https://github.com/monero-ecosystem/monero-rpc-rs/compare/master...refactor-ring:rpc-client-builder-merged?expand=1

UPDATE
Had to change the github actions setup a bit to make the tests pass with the new containers, instead of redefining the containers in the github actions config it now spins up the containers through docker-compose up using the existing docker-compose.yml

@refring refring changed the title Rpc authentication RPC authentication Oct 17, 2022
@silverpill
Copy link
Contributor

+1 for builder

Also it would be nice to have a feature flag for this, to avoid pulling diqwest if you don't need it.

@h4sh3d
Copy link
Member

h4sh3d commented Oct 31, 2022

Also it would be nice to have a feature flag for this, to avoid pulling diqwest if you don't need it.

Agree, a feature would be nice to only pull dependencies when needed.

Given that this one, #77 and #81 are all changing the way we could instantiate a client, a builder pattern that contains everything is the best approach IMO.

@TheCharlatan
Copy link
Member

Also much in favour of using the builder pattern.

@refring
Copy link
Contributor Author

refring commented Nov 13, 2022

Added a feature "rpc_authentication" however when using the "dep:diqwest" notation in cargo.toml the tests fail because of using rust 1.56 and the "dep:" syntax only got added in rust 1.60.

Also, should I make a separate PR for the builder pattern ?

@TheCharlatan
Copy link
Member

Also, should I make a separate PR for the builder pattern ?

I think that would make things a bit easier to review, yes. I'll let @h4sh3d weigh in first on bumping the msrv.

@h4sh3d h4sh3d added this to the v0.3.0 milestone Dec 9, 2022
@h4sh3d h4sh3d linked an issue Dec 9, 2022 that may be closed by this pull request
@h4sh3d
Copy link
Member

h4sh3d commented Dec 12, 2022

Included in #92

@h4sh3d h4sh3d closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add RPC authentification
4 participants