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

api v2 #2

Merged
merged 7 commits into from Mar 12, 2019

Conversation

Projects
None yet
3 participants
@bddap
Copy link
Contributor

commented Feb 26, 2019

This pr adds the ability to generate jsonrpc handlers for APIOwner and APIForeign via proceedural macro.

Since the handler generator is not specific to grin I've split it into its own crate, easy-jsonrpc. easy-jsonrpc provides a macro, jsonrpc_server, which decorates a trait definition and generates appropriate jsonrpc handlers.

This pr introduces a new trait, OwnerAPI, which the existing struct APIOwner implements. A new trait was added for several reasons:

  1. APIOwner contains methods that don't make sense in a jsonrpc context; method signatures needed to be changed.
    • Mutable out parameters don't work in an rpc
    • Functions involving callbacks don't work in an rpc
    • Rpc methods can't return Result<, Error> because Error is not serializable. Result<, ErrorKind> is used instead
    • fn new() never needs to be called from a remote client
  2. Adding a new trait is not a breaking change. Changing APIOwner to an rpc compliant trait would require refactoring other parts of grin-wallet.
  3. APIOwner can trivially implement OwnerAPI (perhaps WalletBackend could implement it as well)

TODO

  • Add doctests for every OwnerAPI method, OwnerAPI doctests will serve as jsonrpc documentation. See OwnerAPI::accounts for an example.
  • Add links to docs for APIOwner methods from each OwnerAPI method doc
  • Implement fn initiate_tx(..) -> Result<(Slate, OutputLockFn<W, C, K>), Error> without using a callback
  • Implement fn tx_lock_outputs(.. , mut lock_fn: OutputLockFn<W, C, K>) -> Result<(), Error> without using a callback
  • Implement issue_send_tx like in refwallet/src/controller.rs
  • Either A) Depreciate APIOwner in favor of OwnerAPI, or B) Rename OwnerAPI to be more decriptive e.g. RPCAPIOwner
  • Add ForeignAPI trait
  • Add jsonrpc_client macro for typed client generation
  • Possibly automatically generate clients in other languages such as javascript

Blockers

This pr requires mimblewimble/grin#2596, which implements serde for error types.

@bddap

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

This pr implements this apiv2 proposal.

This implementation uses option 1. (see proposal) for error reporting. I tried to implement option 3., but it turns out negative trait bounds would be required for differentiating result return types from non-result return types. Negative trait bounds are still a work in progress in rustc.

@bddap

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Again, this pr relies on mimblewimble/grin#2596, so compilation will fail until that pr is accepted.

@yeastplume

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Looks really good so far, very happy to see progress here and I'm going to keep checking out and testing locally as you go to provide more detailed feedback. Will have a think about the APIOwner naming.

I think Igno indicated that we'll be addressing mimblewimble/grin#2596 once a couple of other things are in there.

APIv2 Rewrite first draft
Add WIP JSON RPC handler for APIOwner via proc-macro generated API, using the easy-jsonrpc crate.

@bddap bddap force-pushed the bddap:bddap/apiv2 branch from 55fffc4 to 6402e67 Mar 4, 2019

@yeastplume

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

rust-secp has been updated now

@yeastplume

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@bddap I've just completed some other chunks of work, so the V2 API is becoming the largest thing on my radar at the moment. Would you mind getting this to a state where it can be merged, then we can take that TODO list and turn it into a meta-issue and start tackling the work in parallel?

@bddap

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@yeastplume sounds great. Glad to have help on this. I'll work on getting this to a mergeable state. Can you look into mimblewimble/grin#2659? It needs to be merged before this.

@yeastplume yeastplume referenced this pull request Mar 8, 2019

Open

[META] Wallet Roadmap #11

@graemes

This comment has been minimized.

Copy link

commented Mar 10, 2019

@yeastplume Would there be value in make the api compatible with the appropriate parts of the open banking api's (https://openbanking.atlassian.net/wiki/spaces/DZ/pages/23363964/Read+Write+Data+API+Specifications) The banking sector in the UK is being forced to provide these apis and most other countries are heading down a similar route. Could simplify adoption by wallet providers?

@yeastplume

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@bddap mimblewimble/grin#2659 was merged so let me know if there's anything else you need

@graemes We decided that using JSON-RPC is more appropriate than providing rest APIs, and I don't think the crypto space in general is too concerned with banking data standards (given there are many challenges unique to crypto that wouldn't be addressed by them) so I don't think we'd gain anything by deciding to adhere to them. If anyone felt strongly about it I'm sure they could wrap our JSON-RPC api to conform.

@bddap

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@yeastplume this pr is ready to merge.

Couple more things to do post-merge

Some OwnerApi/ForeignApi doctests are kinda BS at the moment. They expect errors because the NodeClient they are testing against is not running.

  • Spin up a temporary NodeClient for OwnerApi/ForeignApi doctests.

Since we are updating APIs, now is a good time to step back and examine OwnerApi/ForeignApi methods. Which ones can we get rid of? Which ones should we change. @yeastplume you are probably well suited to that task.

  • redesign network APIs if helpful

@bddap bddap changed the title [WIP] api v2 api v2 Mar 12, 2019

@yeastplume

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@bddap Thanks, I'll merge this now then create another meta issue with all of the tasks

The doctests were written when everyone is in the same crate, and including them now involves circular dependencies which we can't have due to crates.io requirements. I'd actually started an empty node client at some stage to try to get around it, will leave the task up as a todo

@yeastplume yeastplume merged commit 3c82b11 into mimblewimble:master Mar 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.