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.

Sign up for GitHub

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

Jump to bottom

Feat/drand-mainnet #27

Closed
wants to merge 2 commits into from
Closed

Feat/drand-mainnet #27

wants to merge 2 commits into from

Conversation

driemworks
Copy link
Contributor

@driemworks driemworks commented Nov 12, 2024 •
edited
Loading

This PR closes #30

  • adds support for drand's mainnet:
    • creates new verifier to verify drand pulses
    • adds a new 'mainnet' feature
  • points to cloudflare API for better consistency
  • also reorganizes some of the code for better maintability

juangirini
juangirini requested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

I think we are abusing feature flags. It is more scalable to use enums to choose the net.
Test-wise we could have two mocks, one for each configuration

pallets/drand/Cargo.toml
Comment on lines +89 to +90
# use the drand mainnet configuration
mainnet = []
Copy link
Contributor

Choose a reason for hiding this comment

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

As per a comment below, I don't think we should specify the net as a feature, but as a pallet config

Suggested change
# use the drand mainnet configuration
mainnet = []

pallets/drand/src/lib.rs
Comment on lines +455 to +462
fn get_chain_hash() -> &'static str {
if cfg!(not(feature = "mainnet")) {
return QUICKNET_CHAIN_HASH;
}

MAINNET_CHAIN_HASH
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not scale well. I would suggest having an enum with the supported versions of drand quicknet and mainnet so far. And pass the value we want to use in the pallet configuration.

pallets/drand/src/lib.rs
/// the main drand api endpoint
pub const API_ENDPOINT: &str = "https://api.drand.sh";
pub const API_ENDPOINT: &str = "https://drand.cloudflare.com";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move the endpoint to the config. I have created a follow-up issue for that

Suggested change
pub const API_ENDPOINT: &str = "https://drand.cloudflare.com";
// TODO: move to config https://github.com/ideal-lab5/pallets/issues/28
pub const API_ENDPOINT: &str = "https://drand.cloudflare.com";

pallets/drand/src/tests.rs
@@ -302,25 +337,25 @@ fn can_execute_and_handle_valid_http_responses() {
let mut state = state.write();
state.expect_request(PendingRequest {
method: "GET".into(),
uri: "https://api.drand.sh/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/info".into(),
response: Some(QUICKNET_INFO_RESPONSE.as_bytes().to_vec()),
uri: "https://drand.cloudflare.com/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/info".into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to unhardcode this. Not necessarily in this PR, but maybe

Suggested change
uri: "https://drand.cloudflare.com/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/info".into(),
// TODO: use fetch_drand_chain_info in tests instead of hardcoded urls https://github.com/ideal-lab5/pallets/issues/28
uri: "https://drand.cloudflare.com/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/info".into(),

pallets/drand/src/tests.rs
sent: true,
..Default::default()
});
state.expect_request(PendingRequest {
method: "GET".into(),
uri: "https://api.drand.sh/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/public/latest".into(),
response: Some(DRAND_RESPONSE.as_bytes().to_vec()),
uri: "https://drand.cloudflare.com/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/public/latest".into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uri: "https://drand.cloudflare.com/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/public/latest".into(),
// TODO: use fetch_drand_chain_info in tests instead of hardcoded urls https://github.com/ideal-lab5/pallets/issues/28
uri: "https://drand.cloudflare.com/52db9ba70e0cc0f6eaf7803dd07447a1f5477735fd3f661792ba94600c84e971/public/latest".into(),

pallets/drand/src/types.rs
/// the round number to track rounds of the beacon
pub type RoundNumber = u64;

/// the expected response body from the drand api endpoint `api.drand.sh/{chainId}/info`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// the expected response body from the drand api endpoint `api.drand.sh/{chainId}/info`
/// the expected response body from the drand api endpoint

pallets/drand/src/types.rs
}

impl BeaconInfoResponse {
/// the default configuration fetches from quicknet
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// the default configuration fetches from quicknet
/// the default configuration fetches from drand

pallets/drand/src/types.rs
}

/// a pulse from the drand beacon
/// the expected response body from the drand api endpoint `api.drand.sh/{chainId}/public/latest`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// the expected response body from the drand api endpoint `api.drand.sh/{chainId}/public/latest`
/// the expected response body from the drand api endpoint

pallets/drand/substrate-node-template/runtime/src/lib.rs
@@ -290,7 +290,7 @@ impl pallet_drand::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type WeightInfo = pallet_drand::weights::SubstrateWeight<Runtime>;
type AuthorityId = pallet_drand::crypto::TestAuthId;
type Verifier = pallet_drand::QuicknetVerifier;
type Verifier = crate::pallet_drand::verifier::MainnetVerifier;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add a "NetworkType" config value, we could probably remove this or make it an Option. So by default if you use NetworkType::Quicknet the verifier is QuicknetVerifier but the implementor could add their own verifier if they want to

@driemworks driemworks marked this pull request as draft November 18, 2024 18:16
@driemworks
Copy link
Contributor Author

Closing for now, will address in the future. See #30

@driemworks driemworks closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juangirini juangirini juangirini requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pallet: pallet_randomness_beacon refactor
2 participants
@driemworks @juangirini