-
Notifications
You must be signed in to change notification settings - Fork 702
[Feature] Leo Devnode CLI command #29012
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
base: master
Are you sure you want to change the base?
Conversation
| tag = "v4.4.0" | ||
| features = [ "test_consensus_heights" ] | ||
| rev = "7787e6e" | ||
| features = ["dev_skip_checks", "test_consensus_heights", "test_targets"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this will always disable checks in the devnode.
Can we enable dev_skip_checks only if the Leo build feature flag devnode_skip_checks is enabled, so developers can also use the devnode with checks enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe you can also write some comments giving context to future developers:
// We set test_consensus_heights to give developers freedom to set custom consensus version upgrade heights.
// We set test_targets because the devnode's genesis block was created with the same feature flag
| // Prepare the new block. | ||
| let new_block = rest | ||
| .ledger | ||
| .prepare_advance_to_next_beacon_block(&private_key, vec![], vec![], txs, &mut rand::thread_rng()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit funky that all transactions will be pushed into the first block. But I don't think we should necessarily change it. Maybe add a comment for clarity.
vicsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides a few nits!
leo/cli/commands/devnode/mod.rs
Outdated
| } | ||
|
|
||
| // A helper function to handle the Devnode command based on the subcommand provided. | ||
| fn hanlde_devnode(devnode_command: LeoDevnode, context: Context) -> Result<<LeoDevnode as Command>::Output> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fn hanlde_devnode(devnode_command: LeoDevnode, context: Context) -> Result<<LeoDevnode as Command>::Output> { | |
| fn handle_devnode(devnode_command: LeoDevnode, context: Context) -> Result<<LeoDevnode as Command>::Output> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed: 77c4102
leo/cli/commands/devnode/mod.rs
Outdated
| } | ||
|
|
||
| fn apply(self, context: Context, _: Self::Input) -> Result<Self::Output> { | ||
| hanlde_devnode(self, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| hanlde_devnode(self, context) | |
| handle_devnode(self, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed: 77c4102
| ] | ||
| license = "GPL-3.0" | ||
| edition = "2024" | ||
| rust-version = "1.92.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we able to keep using 1.92.0? Any reason we downgraded?
| default-features = false | ||
|
|
||
| [workspace.dependencies.rayon] | ||
| version = "1.11.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, are we able to keep using the newer version of rayon rather than downgrading?
| /// An enum of error handlers for the REST API server. | ||
| #[derive(Debug)] | ||
| pub enum RestError { | ||
| /// 400 Bad Request - Invalid input, malformed parameters, validation errors | ||
| BadRequest(AnyhowError), | ||
| /// 404 Not Found - Resource not found | ||
| NotFound(AnyhowError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also consider using thiserror instead of raw anyhow for the RestError. Since this is a structured error type with distinct variants (rather than ad-hoc error propagation), thiserror might be a better fit:
#[derive(Debug, thiserror::Error)]
pub enum RestError {
#[error("Bad request: {0}")]
BadRequest(#[source] AnyhowError),
#[error("Not found: {0}")]
NotFound(#[source] AnyhowError),
#[error("Unprocessable entity: {0}")]
UnprocessableEntity(#[source] AnyhowError),
#[error("Too many requests: {0}")]
TooManyRequests(#[source] AnyhowError),
#[error("Internal server error: {0}")]
InternalServerError(#[source] AnyhowError),
}This would give you Display and std::error::Error for free, while still keeping AnyhowError inside for error chain functionality. The constructor methods (lines 54-77) could then be removed as optional boilerplate.
Not a blocker, just a thought!
| { | ||
| return Err(RestError::too_many_requests(anyhow!("{err_msg}"))); | ||
| } | ||
| println!("We've arrived at the transaction check section."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking if this was left from local debugging or if it's meant to be in the CLI output? I think it should be removed or converted to tracing::debug!.
| impl From<&str> for RestError { | ||
| fn from(msg: &str) -> Self { | ||
| // Default to 500 Internal Server Error | ||
| Self::InternalServerError(anyhow::anyhow!(msg.to_string())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anyhow! accepts &str directly, so this can just be:
| Self::InternalServerError(anyhow::anyhow!(msg.to_string())) | |
| Self::InternalServerError(anyhow::anyhow!("{msg}")) |
JoshuaBatty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really excited for this feature. Spun it up locally and ran through the multisig example end-to-end. Iteration speed feels really good. Just left a few minor nits. Nice work!
Motivation
Devnode is a new tool that creates a local development network to enable developers to rapidly test and iterate through Aleo program design.
leo devnode has two commands:
leo devnode start --private-key APrivateKey1zkp8CZNn3yeCseEtxuVPbDCwSyhGW6yZKUYKfgXmcpoGPWHwhich creates a local network at localhost:3030 andleo devnode advancewhich creates a block and adds it to the ledger.leo devnode advancetakes an optional value n which advances the ledger by n blocks. Default value is 1.Furthermore, this PR adds a
--skip-execute-proofflag for leo execute commands along with a--skip-deploy-certificatefor program deployment transactions.JS SDK tests
Reviewers are welcome to use the JS SDK Devnode support methods in this script: https://github.com/ProvableHQ/sdk/tree/mainnet/create-leo-app/template-devnode-js