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

Wallet API V2 - Suggestions #46

Closed
yeastplume opened this issue Nov 29, 2018 · 6 comments
Closed

Wallet API V2 - Suggestions #46

yeastplume opened this issue Nov 29, 2018 · 6 comments

Comments

@yeastplume
Copy link
Member

The current HTTP/json wallet API wasn't really designed in any way, it grew over time and currently doesn't do the best job at things it should be doing, such as exposing function parameters and returning relevant error messages. While a lot of changes to the 'real' underlying APIs have occurred recently, I haven't made any changes to the json API wrapper to allow existing applications continue to work as before.

I'm considering what to do for a version 2, and I think however it's built it should simply be a json wrapper around the 'real' Owner and Foreign APIs, which you'd get directly if you linked code against grin_wallet, are defined here (starting to document them properly): https://github.com/mimblewimble/grin/blob/master/wallet/src/libwallet/api.rs

Doing it this way has a few advantages, in that the same documentation can be used (the JSON API documentation can just point to the rustdoc generated API and will be more consistency regardless of how apps are linked.

I'm currently leaning towards using JSON RPC instead of the current RESTy-type API that's there are the moment. The wallet doesn't operate in a very CRUDdy fashion, and I think JSON RPC suits the workflow much better, is easier to manage and is much more suitable for handling and returning appropriate errors. I'd really like this layer to be a thin wrapper, would love it even more if there were a way to automatically generate it and ensure the documentation is generated directly from the code.

So as not to break existing compatibility for launch, I'd probably prefer to leave the existing API in place for now, and have the new version of the API on another endpoint. We'd then retire the older API at a later date.

These are my current thoughts on what should happen here, throwing this open as I'm quite happy to take suggestions or pointers as to what the V2 wallet HTTP/JSON API should look like.

@ignopeverell
Copy link

Overall I do think the most important is making it easy to integrate and both JSONy or JSON/RPC are rather simple (although the latter can't be consumed straight from a browser). So prioritizing for ease of maintenance for documentation makes sense.

@yeastplume
Copy link
Member Author

You'd think this is something that could be automated via procedural macros, which should just read in each function's syntax tree, create a new one corresponding to the json wrapper, then spit it back out again. Any libs I've seen around that hint at doing this tend to be part of some heavyweight hyper or iron based framework. Does anyone know of a lightweight lib that does something like this?

@bddap
Copy link
Contributor

bddap commented Jan 9, 2019

https://github.com/paritytech/jsonrpc may be helpful here. It includes macros which are somewhat similar to what @yeastplume is describing.

It's posted as jsonrpc-core on crates.io.

@bddap
Copy link
Contributor

bddap commented Jan 15, 2019

After some experimentation, I found that jsonrpc-core is nearly viable, but the macro they currently use is old-style declarative and pretty flakey. It attempts to manually parse a trait definition which is no longer necessary now that procedural macros are stable. There is an active, in-progress pr to add a new custom derive using proc macros and syn.

Until the new custom derive is complete, I think it would be most efficient to use jsonrpc-core without macros. Later transitioning to an automatically generated jsonrpc api will be fairly straightforward.

@ascjones
Copy link

@quentinlesceller
Copy link
Member

@lehnberg lehnberg transferred this issue from mimblewimble/grin Apr 16, 2019
garyyu added a commit to garyyu/grin-wallet that referenced this issue Aug 5, 2019
* minor change on logs

* rustfmt
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

5 participants