Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Adds babe rpc support #4729

Merged
merged 11 commits into from
Feb 17, 2020
Merged

Adds babe rpc support #4729

merged 11 commits into from
Feb 17, 2020

Conversation

seunlanlege
Copy link
Contributor

@seunlanlege seunlanlege commented Jan 24, 2020

Adds the babe_epochAuthorship rpc method for querying information about slots that can be claimed in the current epoch.

closes #4102

@seunlanlege seunlanlege added A3-in_progress Pull request is in progress. No review needed at this stage. M4-core labels Jan 24, 2020
@parity-cla-bot
Copy link

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@seunlanlege seunlanlege marked this pull request as ready for review January 29, 2020 17:20
primitives/consensus/babe/src/lib.rs Outdated Show resolved Hide resolved
primitives/consensus/babe/src/digest.rs Outdated Show resolved Hide resolved
primitives/consensus/babe/src/digest.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Overall logic is good, just some nits regarding what data needs to be exposed and how to format the result. I'm not super familiar with the RPC subsystem but where are wiring this up in the node implementation?

primitives/consensus/babe/src/digest.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/rpc.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/rpc.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/rpc.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/rpc.rs Outdated Show resolved Hide resolved
@seunlanlege seunlanlege force-pushed the seun-babe-rpc branch 2 times, most recently from 6d539dd to de99a5f Compare February 6, 2020 15:51
@seunlanlege seunlanlege added B1-clientnoteworthy and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 6, 2020
@seunlanlege
Copy link
Contributor Author

looks like faulty code was merged into master.

error[E0624]: method `create_address` is private
   --> frame/evm/src/lib.rs:348:34
    |
348 |             let create_address = executor.create_address(source, evm::CreateScheme::Dynamic);
    |                                           ^^^^^^^^^^^^^^
error: aborting due to previous error
For more information about this error, try `rustc --explain E0624`.
error: could not compile `pallet-evm`.

@seunlanlege seunlanlege force-pushed the seun-babe-rpc branch 5 times, most recently from 842772b to eb1b2bc Compare February 11, 2020 17:52
@rphmeier
Copy link
Contributor

Why is the Cargo.lock diff so large?

@expenses
Copy link
Contributor

@rphmeier it's using the old, no-so-git-friendly style of Cargo.lock. @seunlanlege please run git checkout parity/master -- Cargo.lock and then cargo check to re-format it.

bkchr
bkchr previously requested changes Feb 11, 2020
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Please no test-helpers features in Substrate. Features are broken in Cargo/Rust.

When you enable the feature, it will leak into the main build.

client/keystore/Cargo.toml Outdated Show resolved Hide resolved
client/keystore/src/testing.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link
Contributor Author

@andresilva could you please take a look at this again?

bin/node/rpc/src/lib.rs Outdated Show resolved Hide resolved
bin/node/rpc/src/lib.rs Show resolved Hide resolved
client/consensus/babe/Cargo.toml Outdated Show resolved Hide resolved
client/consensus/babe/src/rpc.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/rpc.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/tests.rs Outdated Show resolved Hide resolved
bin/node/cli/src/service.rs Show resolved Hide resolved
bin/node/cli/src/service.rs Show resolved Hide resolved
bin/node/cli/src/service.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link
Contributor Author

Ping @tomusdrw , @andresilva

This should also be ready for merging.

@andresilva
Copy link
Contributor

I will give this another review in the afternoon. Did you test how this works in the actual node binary? (e.g. on dev chain we should get all slots).

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

client/consensus/babe/src/lib.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link
Contributor Author

Did you test how this works in the actual node binary?

➜ curl -v --data '{"jsonrpc":"2.0","method":"babe_epochAuthorship","params":[], "id": 1}' -H 'Content-Type:application/json'  http://127.0.0.1:9933
* Rebuilt URL to: http://127.0.0.1:9933/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 9933 (#0)
> POST / HTTP/1.1
> Host: 127.0.0.1:9933
> User-Agent: curl/7.58.0
> Accept: */*
> Content-Type:application/json
> Content-Length: 70
> 
* upload completely sent off: 70 out of 70 bytes
< HTTP/1.1 200 OK
< content-type: application/json; charset=utf-8
< content-length: 2115
< date: Fri, 14 Feb 2020 15:15:19 GMT
< 
{"jsonrpc":"2.0","result":{"5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY":{"primary":[527231097,527231099,527231101,527231103,527231104,527231107,527231113,527231126,527231127,527231131,527231137,527231138,527231140,527231142,527231144,527231145,527231146,527231148,527231149,527231150,527231152,527231155,527231161,527231171,527231173,527231175,527231178,527231181,527231182,527231188,527231189,527231192,527231194,527231197,527231200,527231207,527231216,527231224,527231241,527231245,527231248,527231250,527231253,527231258,527231260,527231263,527231281,527231284,527231288,527231289,527231290,527231293,527231294],"secondary":[527231095,527231096,527231098,527231100,527231102,527231105,527231106,527231108,527231109,527231110,527231111,527231112,527231114,527231115,527231116,527231117,527231118,527231119,527231120,527231121,527231122,527231123,527231124,527231125,527231128,527231129,527231130,527231132,527231133,527231134,527231135,527231136,527231139,527231141,527231143,527231147,527231151,527231153,527231154,527231156,527231157,527231158,527231159,527231160,527231162,527231163,527231164,527231165,527231166,527231167,527231168,527231169,527231170,527231172,527231174,527231176,527231177,527231179,527231180,527231183,527231184,527231185,527231186,527231187,527231190,527231191,527231193,527231195,527231196,527231198,527231199,527231201,527231202,527231203,527231204,527231205,527231206,527231208,527231209,527231210,527231211,527231212,527231213,527231214,527231215,527231217,527231218,527231219,527231220,527231221,527231222,527231223,527231225,527231226,527231227,527231228,527231229,527231230,527231231,527231232,527231233,527231234,527231235,527231236,527231237,527231238,527231239,527231240,527231242,527231243,527231244,527231246,527231247,527231249,527231251,527231252,527231254,527231255,527231256,527231257,527231259,527231261,527231262,527231264,527231265,527231266,527231267,527231268,527231269,527231270,527231271,527231272,527231273,527231274,527231275,527231276,527231277,527231278,527231279,527231280,527231282,527231283,527231285,527231286,527231287,527231291,527231292]}},"id":1}
* Connection #0 to host 127.0.0.1 left intact

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm. a bunch of docs/formatting nits.

bin/node/rpc/src/lib.rs Outdated Show resolved Hide resolved
bin/node/rpc/src/lib.rs Show resolved Hide resolved
bin/node/rpc/src/lib.rs Outdated Show resolved Hide resolved
bin/node/rpc/src/lib.rs Outdated Show resolved Hide resolved
bin/node/rpc/src/lib.rs Outdated Show resolved Hide resolved
client/consensus/babe/rpc/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link
Contributor Author

looks like all is well, cc @bkchr

@andresilva andresilva merged commit c50844a into master Feb 17, 2020
@andresilva andresilva deleted the seun-babe-rpc branch February 17, 2020 18:05
@andresilva
Copy link
Contributor

Thanks @seunlanlege! :)

General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Feb 18, 2020
* babe_epochAuthorship
remove test-helpers from sp-keyring, bump spec_version, impl_version

* bump Cargo.lock

* add BabeRPC to node-rpc

* rename to BabeApi, remove err_derive

* pass &ServiceBuilder to with_rpc_extensions callback

* sc-consensus-babe-rpc

* Update client/consensus/babe/src/lib.rs

Co-Authored-By: Tomasz Drwięga <[email protected]>

* Better docs, code style chanegs

Co-Authored-By: André Silva <[email protected]>

* new line at the end of Cargo.toml

Co-authored-by: Tomasz Drwięga <[email protected]>
Co-authored-by: André Silva <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method to get BABE block authorship information
8 participants