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

Node API v2 #3094

Merged
merged 9 commits into from Dec 6, 2019
Merged

Node API v2 #3094

merged 9 commits into from Dec 6, 2019

Conversation

quentinlesceller
Copy link
Member

@quentinlesceller quentinlesceller commented Oct 11, 2019

Replacement of #3077 does not introduce any backward incompatibility.

RFC here with more details mimblewimble/grin-rfcs#28
TLDR: Introduce a Node JSON-RPC API v2.

TODO:

  • get_status
  • get_version
  • get_block
  • get_header
  • chain methods
  • pool methods
  • peers methods
  • txhashset methods
  • Working documentation on docs.rs
  • Doctest In a different PR
  • Internal use of API v2 for GrinIn a different PR

BONUS:
ban_peer/unban_peer can be port specific.
peers.ban_peer/peers.unban_peer now return Result<(),Error>

how to test this PR

git remote add quentinlesceller git://github.com/quentinlesceller/grin.git
git fetch quentinlesceller
git checkout quentinlesceller/nodeapi
cargo build --release

Run grin and do POST queries with JSON-RPC body to http://127.0.0.1:3413/v2/owner and http://127.0.0.1:3413/v2/foreign.

@lehnberg lehnberg modified the milestones: tentative 3.0.0, 3.0.0 Oct 30, 2019
@lehnberg lehnberg added the P2: Important Required for a release label Oct 30, 2019
@quentinlesceller
Copy link
Member Author

Latest commit introduces a lot of things:

  • Owner API:

    • ban_peer
    • unban_peer
    • get_connected_peers
    • get_peers
    • compact_chain
    • validate_chain
    • get_status
  • Foreign API:

    • push_transaction
    • get_unconfirmed_transactions
    • get_stempool_size
    • get_pool_size
    • get_pmmr_indices
    • get_unspent_outputs
    • get_outputs
    • get_kernel
    • get_tip
    • get_version
    • get_block
    • get_header
  • Basic auth for Owner API is the same as the V1 basic auth (specified in .api_secret)

  • Basic auth for Foreign API is different and set in .foreign_api_secret.

api/src/types.rs Outdated
@@ -266,7 +266,7 @@ pub struct OutputPrintable {
/// Rangeproof hash (as hex string)
pub proof_hash: String,
/// Block height at which the output is found
pub block_height: Option<u64>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change here? Just wondering if anything in the wallet will be expecting a ‘null’ here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember why I changed the structure from Option<u64>to u64. Reverting.

@yeastplume
Copy link
Member

First, all looks very good and well done. I’m glad to see the approach done in the wallet duplicated here cause I think it works quite well.

Am okay with this in general, particularly as it runs alongside the v1 api which should give us time to adapt and asjust. There’s the obvious caveat that there may be some further changes required as I haven’t had any chance to test the behaviour of needed wallet functions against the V2 API.

Also, definitely missing a trick here by not having doctest functions with sample JSON for each function. This should be easy to do, however it would likely require test framework code that currently only exists in the wallet. We really need to figure our our overall integration strategy, but that’s another discussion.

Good job here, and look forward to porting the wallet functions at some point between v3.0.0 and v4.0.0

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested V1 api and everything looks good there.
Tested grin server config and confirmed the node will start up with the default config file.

I'm good with this.

👍

@antiochp antiochp merged commit cdb2d6c into mimblewimble:master Dec 6, 2019
@quentinlesceller quentinlesceller deleted the nodeapi branch December 6, 2019 11:01
@antiochp antiochp mentioned this pull request Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement P2: Important Required for a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants