Move odra cli to the main repo #515
Conversation
WalkthroughThe pull request introduces several updates across multiple files, primarily focusing on the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Outside diff range and nitpick comments (12)
odra-cli/Cargo.toml (1)
6-20: Dependencies look good, with minor suggestions for improvement.The dependencies are well-chosen for a CLI tool interacting with smart contracts. However, consider the following suggestions:
For better reproducibility, consider using more specific version constraints for some dependencies. For example:
anyhow = "1.0.86"→anyhow = "1.0.86"thiserror = "1.0.30"→thiserror = "1.0.30"hex = "0.4.3"→hex = "0.4.3"For
serdeand related crates, consider using the same version across all three to ensure compatibility:serde = { version = "1.0.197", default-features = false } serde_json = { version = "1.0.114", default-features = false } serde_derive = { version = "1.0.197", features = ["derive"] }For
chrono, consider specifying a more precise version:chrono = { version = "0.4.35", features = ["serde"] }These changes will help ensure consistent behavior across different environments and make it easier to reproduce builds.
odra-cli/src/cmd/deploy.rs (3)
11-13: Improve documentation comments for clarity and adherence to Rust conventionsThe documentation comments for
DeployCmdcan be more concise and follow Rust's conventions for doc comments. Consider rephrasing to eliminate redundancy and enhance clarity.Apply this diff to update the comments:
/// Represents the deploy command in the Odra CLI. /// -/// The deploy command runs the [DeployScript]. +/// Executes the associated [DeployScript].
35-39: Simplify the return type by importingResultIn the
DeployScripttrait, you can simplify the return type by importingResultfrom the standard library rather than using the fully qualified path.Apply this diff:
use thiserror::Error; +use std::result::Result; + pub trait DeployScript { fn deploy( &self, env: &HostEnv, container: &mut DeployedContractsContainer - ) -> core::result::Result<(), DeployError>; + ) -> Result<(), DeployError>; }
51-57: Remove unnecessaryimpl From<OdraError> for DeployErrorAfter including
OdraErrordirectly inDeployErrorwith the#[from]attribute, the manual implementation ofFrom<OdraError>becomes redundant. You can safely remove it to clean up the code.Apply this diff to remove the redundant implementation:
impl From<OdraError> for DeployError { fn from(err: OdraError) -> Self { - DeployError::OdraError { - message: format!("{:?}", err) - } + // This implementation is no longer necessary due to the #[from] attribute. } }odra-cli/src/entry_point.rs (1)
50-51: Clarify the condition foruse_proxyThe condition used to determine
use_proxymay not be immediately clear to readers.Consider adding a comment to explain why
use_proxyis set based on the return type and the attached amount.// Use proxy if the method returns a value or if an amount is attached let use_proxy = ty.0 != NamedCLType::Unit || !call_def.amount().is_zero();odra-cli/src/cmd/contract.rs (1)
67-74: Remove unnecessary variable bindings inCallCmd::runThe local variables
entry_pointandcontract_nameare directly referencing struct fields without modification. You can streamline the code by using the fields directly.Apply this diff:
fn run(&self, env: &HostEnv, args: &ArgMatches, types: &CustomTypeSet) -> Result<()> { - let entry_point = &self.entry_point; - let contract_name = &self.contract_name; - let result = entry_point::call(env, contract_name, entry_point, args, types)?; + let result = entry_point::call(env, &self.contract_name, &self.entry_point, args, types)?; prettycli::info(&result); Ok(()) }This makes the code more concise without altering functionality.
odra-cli/src/cmd/scenario.rs (2)
84-84: Consider restricting the visibility ofScenarioArgsIf
ScenarioArgsis intended for internal use within the module or crate, consider reducing its visibility to prevent unintended usage and to encapsulate internal implementation details.Apply this diff:
-pub struct ScenarioArgs(HashMap<String, ScenarioArg>); +pub(crate) struct ScenarioArgs(HashMap<String, ScenarioArg>);
175-178: Add documentation comments forScenarioMetadatatraitPublic traits should have documentation comments to explain their purpose and usage. Adding these comments improves code readability and assists other developers in understanding how to implement the trait.
Apply this diff:
/// ScenarioMetadata is a trait that represents the metadata of a scenario. +/// +/// Implement this trait to provide the name and description of your custom scenario. +/// This metadata is used by the CLI to display information about the scenario. pub trait ScenarioMetadata { const NAME: &'static str; const DESCRIPTION: &'static str; }odra-cli/src/args.rs (4)
57-57: Usevalue_parserfor argument type validationIn the
From<CommandArg>implementation, consider adding.value_parserto enforce type parsing at the command-line level, improving user experience by catching invalid inputs early.Apply this diff to include a value parser based on the argument type:
let result = Arg::new(&arg.name) .long(arg.name) .value_name(format!("{:?}", arg.ty)) + .value_parser(clap::builder::ValueParser::new(ValueParser::from(&arg.ty))) .required(arg.required) .help(arg.description);
268-272: Combine conditions to simplify control flowIn the
build_complex_argfunction, lines 266~ and 268~ handle similar logic for list elements and non-list elements. Consider simplifying the control flow for better readability.Apply this diff to combine conditions:
} else if current_group.name == parent && is_list_element { current_group.add((ty, args)); } else { - current_group.flush(&mut buffer)?; - let bytes = types::into_bytes(&ty, args[0])?; - buffer.extend_from_slice(&bytes); + if !current_group.name.is_empty() { + current_group.flush(&mut buffer)?; + } + if is_list_element { + current_group.add((ty, args)); + } else { + let bytes = types::into_bytes(&ty, args[0])?; + buffer.extend_from_slice(&bytes); + } }
356-363: Ensure consistency in argument help messagesIn the
attached_value_argfunction, the help message refers to "CSPRs." For clarity and consistency, consider using the same terminology throughout the CLI.For example, if other parts of the CLI refer to the currency as "CSPR tokens," you might update the help message:
Arg::new(ARG_ATTACHED_VALUE) - .help("The amount of CSPRs attached to the call") + .help("The amount of CSPR tokens attached to the call")
357-362: Specify the value type for better CLI experienceTo enhance the command-line interface, specify the expected value type using
.value_parserfor theattached_value_arg.Apply this diff to include the value parser:
Arg::new(ARG_ATTACHED_VALUE) .help("The amount of CSPR tokens attached to the call") .long(ARG_ATTACHED_VALUE) .required(false) .value_name(format!("{:?}", NamedCLType::U512)) + .value_parser(clap::value_parser!(u512)) .action(ArgAction::Set)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- Cargo.toml (2 hunks)
- core/src/host.rs (1 hunks)
- odra-cli/Cargo.toml (1 hunks)
- odra-cli/src/args.rs (1 hunks)
- odra-cli/src/cmd/contract.rs (1 hunks)
- odra-cli/src/cmd/deploy.rs (1 hunks)
- odra-cli/src/cmd/mod.rs (1 hunks)
- odra-cli/src/cmd/scenario.rs (1 hunks)
- odra-cli/src/container.rs (1 hunks)
- odra-cli/src/entry_point.rs (1 hunks)
- odra-cli/src/lib.rs (1 hunks)
- odra-cli/src/test_utils.rs (1 hunks)
- odra-cli/src/types.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (17)
odra-cli/Cargo.toml (1)
1-5: LGTM: Package metadata is well-defined.The package metadata is correctly specified:
- The name "odra-cli" accurately reflects the purpose of the package.
- The version number follows semantic versioning conventions.
- The edition is set to the latest stable Rust edition (2021).
Cargo.toml (3)
11-11: LGTM: Addition of odra-cli to workspace membersThe addition of "odra-cli" to the workspace members is consistent with the PR objective of moving the odra cli to the main repo. The placement and formatting are correct.
33-33: LGTM: Addition of odra-casper-livenet-env dependencyThe addition of odra-casper-livenet-env as a dependency is appropriate. The path and version are correctly specified, and the version (1.3.0) aligns with the workspace package version.
11-11: Summary: Changes align with PR objectiveThe modifications to this Cargo.toml file are consistent with the PR objective of moving the odra cli to the main repo. The addition of "odra-cli" to the workspace members and the inclusion of the odra-casper-livenet-env dependency are appropriate steps for this integration. The changes are well-structured and maintain the existing file format.
Also applies to: 33-33
odra-cli/src/entry_point.rs (3)
1-24: Good usage of custom error handling withCallErrorThe
CallErrorenum provides comprehensive coverage of potential errors, enhancing readability and maintainability. The use ofthiserror::Erroris appropriate and follows best practices.
52-54: Verify gas setting for contract callsCurrently, gas is set only for mutable entry points. Ensure this aligns with the expected behavior of contract execution.
Should gas also be set for immutable (read-only) entry points? If read-only operations consume gas in the environment, consider setting gas accordingly.
58-59: Handle potential decoding errors gracefullyEnsure that any errors during the decoding of the result are properly handled and communicated.
Confirm that
args::decodeprovides meaningful error messages and that these are correctly propagated to the caller.odra-cli/src/cmd/mod.rs (1)
44-60: Well-structured implementation of command delegation.The implementation of
OdraCommandforOdraCliCommandcorrectly delegatesnameandruncalls to the respective command variants. This design promotes modularity and adheres to the command pattern effectively.odra-cli/src/cmd/contract.rs (2)
13-16: Well-structured definition ofContractCmdThe
ContractCmdstruct is appropriately defined with clear fieldsnameandcommands, enhancing maintainability and readability.
71-72: Verify consistent error handling and user feedbackThe output of
entry_point::callis displayed usingprettycli::info(&result);. Ensure that in case of errors, appropriate messages are conveyed to the user to improve the CLI experience.Run the following script to check how errors are handled across the codebase:
This script searches for instances where
entry_point::callis used with the?operator, indicating error propagation, and provides context lines to review error handling practices.odra-cli/src/test_utils.rs (4)
9-30: Functionmock_entry_pointis correctly definedThe
mock_entry_pointfunction is well-defined with appropriate arguments reflecting the expected types. The use ofNamedCLType::Custom("PaymentVoucher".to_string())for thevoucherargument andNamedCLType::List(Box::new(NamedCLType::U8))for thesignatureargument aligns with their definitions.
32-61: Functionmock_command_argsaligns with struct fieldsThe
mock_command_argsfunction accurately creates command arguments corresponding to the fields of thePaymentVoucher,PaymentInfo, andNameMintInfostructs. This ensures that command-line inputs can be correctly mapped to the expected data structures.
63-67: Aggregation of custom types incustom_typesfunctionThe
custom_typesfunction effectively aggregates schema types fromPaymentVoucherandNameTokenMetadata. This is essential for properly handling custom serialization and deserialization of these types within the Odra framework.
69-125: Struct definitions and constructors are properly implementedThe definitions of
NameTokenMetadata,PaymentVoucher,PaymentInfo, andNameMintInfostructs, along with their respectivenewconstructors, are correctly implemented. They provide clear and necessary initializations for the associated data, facilitating reliable test utilities.odra-cli/src/cmd/scenario.rs (1)
Line range hint
1-179: Overall code is well-structured and implements functionality effectivelyThe code presents a clear structure for defining and running custom scenarios within the Odra CLI. The usage of traits and error handling aligns well with Rust best practices.
odra-cli/src/args.rs (1)
311-312:⚠️ Potential issueUse the correct type for enum discriminant
When decoding the enum discriminant, the type is specified as
u8, but the parsed value is asu16. This mismatch could cause issues if the discriminant exceedsu8range.Consider updating the type to match the discriminant size:
let ty = Type(NamedCLType::U8); let (value, rem) = decode(bytes, &ty, types)?; -let discriminant = types::parse_value::<u16>(&value)?; +let discriminant = types::parse_value::<u8>(&value)?;Likely invalid or redundant comment.
core/src/host.rs (1)
338-343: Verify thatevents_countis not accessed before initialization outsideraw_call_contractBy moving the initialization of
events_countinto theraw_call_contractmethod, ensure that no other methods are accessingevents_countassuming it has been initialized during contract registration. This could lead to potential panics or unexpected behavior due to uninitialized values.Run the following script to search for accesses to
events_countoutside ofraw_call_contract:This script will display any lines where
events_countis accessed outside of theraw_call_contractmethod, allowing you to verify that such usages handle uninitialized cases properly.
| .raw_call_contract(contract_address, call_def, use_proxy) | ||
| .map_err(|e| CallError::ExecutionError(format!("{:?}", e)))?; |
There was a problem hiding this comment.
Enhance error reporting from raw_call_contract
Using format!("{:?}", e) may expose internal details and make error messages less user-friendly.
Consider using the error's display implementation or providing a more descriptive message.
.map_err(|e| CallError::ExecutionError(e.to_string()))?;This change ensures that the error message is clearer and more suitable for end-users.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .raw_call_contract(contract_address, call_def, use_proxy) | |
| .map_err(|e| CallError::ExecutionError(format!("{:?}", e)))?; | |
| .raw_call_contract(contract_address, call_def, use_proxy) | |
| .map_err(|e| CallError::ExecutionError(e.to_string()))?; |
| let container = DeployedContractsContainer::load()?; | ||
| let amount = args | ||
| .try_get_one::<String>(ARG_ATTACHED_VALUE) | ||
| .ok() | ||
| .flatten() | ||
| .map(|s| U512::from_dec_str(s).map_err(|_| types::Error::Serialization)) | ||
| .unwrap_or(Ok(U512::zero()))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the retrieval of the attached value
The logic for extracting the amount from the arguments can be streamlined for better readability.
Consider refactoring the code as follows:
let amount = args
.try_get_one::<String>(ARG_ATTACHED_VALUE)
.ok()
.flatten()
- .map(|s| U512::from_dec_str(s).map_err(|_| types::Error::Serialization))
- .unwrap_or(Ok(U512::zero()))?;
+ .map_or_else(
+ || Ok(U512::zero()),
+ |s| U512::from_dec_str(s).map_err(|_| types::Error::Serialization)
+ )?;This refactor uses map_or_else to make the code more concise and eliminates the need for unwrap_or.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let container = DeployedContractsContainer::load()?; | |
| let amount = args | |
| .try_get_one::<String>(ARG_ATTACHED_VALUE) | |
| .ok() | |
| .flatten() | |
| .map(|s| U512::from_dec_str(s).map_err(|_| types::Error::Serialization)) | |
| .unwrap_or(Ok(U512::zero()))?; | |
| let container = DeployedContractsContainer::load()?; | |
| let amount = args | |
| .try_get_one::<String>(ARG_ATTACHED_VALUE) | |
| .ok() | |
| .flatten() | |
| .map_or_else( | |
| || Ok(U512::zero()), | |
| |s| U512::from_dec_str(s).map_err(|_| types::Error::Serialization) | |
| )?; |
| /// OdraCommand is a trait that represents a command that can be run in the Odra CLI. | ||
| pub(crate) trait OdraCommand { | ||
| fn name(&self) -> &str; | ||
| fn run(&self, env: &HostEnv, args: &ArgMatches, types: &CustomTypeSet) -> Result<()>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider relaxing the visibility of the OdraCommand trait.
The OdraCommand trait is currently declared with pub(crate), which restricts its visibility to the current crate. If there's a possibility that external crates might need to implement or use this trait, consider changing it to pub for broader accessibility.
odra-cli/src/types.rs
Outdated
| @@ -0,0 +1,444 @@ | |||
| #![allow(dead_code, unused_variables)] | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the module-level allowance of dead code and unused variables
The attribute #![allow(dead_code, unused_variables)] at the top of the module suppresses warnings for the entire file, which may hide potential issues like unused code or variables that could be cleaned up. It's advisable to apply these allowances more selectively or remove them if they are no longer necessary to maintain code quality.
odra-cli/src/types.rs
Outdated
| let bytes = parts | ||
| .iter() | ||
| .map(|part| parse_hex(input)) |
There was a problem hiding this comment.
Fix incorrect variable usage in hex parsing of byte array elements
Within the parts.iter().map call, the code mistakenly uses parse_hex(input) instead of parse_hex(part). This causes the entire input string to be parsed repeatedly in each iteration rather than parsing each individual part, leading to incorrect behavior.
Apply this diff to correct the error:
- .map(|part| parse_hex(input))
+ .map(|part| parse_hex(part))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let bytes = parts | |
| .iter() | |
| .map(|part| parse_hex(input)) | |
| let bytes = parts | |
| .iter() | |
| .map(|part| parse_hex(part)) |
There was a problem hiding this comment.
Shouldn't this be .map(|part| parse_hex(part))?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| 100, 10, 52, 124, 59, 94, 73 | ||
| ]; | ||
|
|
||
| const NAMED_TOKEN_METADATA_JSON: &str = r#"{ | ||
| "token_hash": "kpob", | ||
| "expiration": "2000000000000", | ||
| "resolver": "Key::Hash(e24a366ebac487e9f3da31af5b8e2a67accd614c5ff73dbc3c640a347c3b5e49)" | ||
| }"#; | ||
|
|
||
| #[test] | ||
| fn test_decode() { | ||
| let custom_types = test_utils::custom_types(); | ||
|
|
||
| let ty = Type(NamedCLType::Custom("NameTokenMetadata".to_string())); | ||
| let (result, _bytes) = | ||
| super::decode(&NAMED_TOKEN_METADATA_BYTES, &ty, &custom_types).unwrap(); | ||
| pretty_assertions::assert_eq!(result, NAMED_TOKEN_METADATA_JSON); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_command_args() { | ||
| let entry_point = test_utils::mock_entry_point(); | ||
| let custom_types = test_utils::custom_types(); | ||
|
|
||
| let args = entry_point | ||
| .arguments | ||
| .iter() | ||
| .flat_map(|arg| super::flat_arg(arg, &custom_types, false)) | ||
| .flatten() | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let expected = test_utils::mock_command_args(); | ||
| pretty_assertions::assert_eq!(args, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_compose() { | ||
| let entry_point = test_utils::mock_entry_point(); | ||
|
|
||
| let mut cmd = Command::new("myprog"); | ||
| test_utils::mock_command_args() | ||
| .into_iter() | ||
| .map(Into::into) | ||
| .for_each(|arg: Arg| { | ||
| cmd = cmd.clone().arg(arg); | ||
| }); | ||
|
|
||
| let args = cmd.get_matches_from(vec![ | ||
| "myprog", | ||
| "--voucher.payment.buyer", | ||
| "hash-56fef1f62d86ab68655c2a5d1c8b9ed8e60d5f7e59736e9d4c215a40b10f4a22", | ||
| "--voucher.payment.payment_id", | ||
| "id_001", | ||
| "--voucher.payment.amount", | ||
| "666", | ||
| "--voucher.names.label", | ||
| "kpob", | ||
| "--voucher.names.owner", | ||
| "hash-f01cec215ddfd4c4a19d58f9c917023391a1da871e047dc47a83ae55f6cfc20a", | ||
| "--voucher.names.token_expiration", | ||
| "1000000", | ||
| "--voucher.names.label", | ||
| "qwerty", | ||
| "--voucher.names.owner", | ||
| "hash-f01cec215ddfd4c4a19d58f9c917023391a1da871e047dc47a83ae55f6cfc20a", | ||
| "--voucher.names.token_expiration", | ||
| "1000000", | ||
| "--voucher.voucher_expiration", | ||
| "2000000", | ||
| "--signature", | ||
| "1,148,81,107,136,16,186,87,48,202,151", | ||
| ]); | ||
| let types = test_utils::custom_types(); | ||
| let args = super::compose(&entry_point, &args, &types).unwrap(); | ||
| let expected = runtime_args! { | ||
| "voucher" => PaymentVoucher::new( | ||
| PaymentInfo::new( | ||
| "hash-56fef1f62d86ab68655c2a5d1c8b9ed8e60d5f7e59736e9d4c215a40b10f4a22", | ||
| "id_001", | ||
| "666" | ||
| ), | ||
| vec![ | ||
| NameMintInfo::new( | ||
| "kpob", | ||
| "hash-f01cec215ddfd4c4a19d58f9c917023391a1da871e047dc47a83ae55f6cfc20a", | ||
| 1000000 | ||
| ), | ||
| NameMintInfo::new( | ||
| "qwerty", | ||
| "hash-f01cec215ddfd4c4a19d58f9c917023391a1da871e047dc47a83ae55f6cfc20a", | ||
| 1000000 | ||
| ) | ||
| ], | ||
| 2000000 | ||
| ), | ||
| "signature" => Bytes::from(vec![1u8, 148u8, 81u8, 107u8, 136u8, 16u8, 186u8, 87u8, 48u8, 202u8, 151u8]), | ||
| }; | ||
| pretty_assertions::assert_eq!(args, expected); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand unit tests to cover edge cases and error handling
The current tests verify standard functionality but may not cover edge cases or invalid inputs. Consider adding tests for scenarios such as missing arguments, invalid types, or decoding errors to ensure robustness.
Would you like assistance in creating additional unit tests to cover these cases?
odra-cli/src/args.rs
Outdated
| #[error("Invalid arg value")] | ||
| TypesError(#[from] types::Error), | ||
| #[error("Decoding error: {0}")] | ||
| DecodingError(String), | ||
| #[error("Arg not found: {0}")] | ||
| ArgNotFound(String), | ||
| #[error("Arg type not found: {0}")] | ||
| ArgTypeNotFound(String) | ||
| } |
There was a problem hiding this comment.
Ensure consistent and informative error messages in ArgsError
The error message for the TypesError variant is "Invalid arg value", but this variant wraps types::Error, which may contain more detailed information. Consider updating the error message to include the source error for better clarity.
Apply this diff to include the underlying error message:
#[derive(Debug, Error)]
pub enum ArgsError {
- #[error("Invalid arg value")]
+ #[error("Invalid arg value: {0}")]
TypesError(#[from] types::Error),
#[error("Decoding error: {0}")]
DecodingError(String),
#[error("Arg not found: {0}")]
ArgNotFound(String),
#[error("Arg type not found: {0}")]
ArgTypeNotFound(String)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[error("Invalid arg value")] | |
| TypesError(#[from] types::Error), | |
| #[error("Decoding error: {0}")] | |
| DecodingError(String), | |
| #[error("Arg not found: {0}")] | |
| ArgNotFound(String), | |
| #[error("Arg type not found: {0}")] | |
| ArgTypeNotFound(String) | |
| } | |
| #[derive(Debug, Error)] | |
| pub enum ArgsError { | |
| #[error("Invalid arg value: {0}")] | |
| TypesError(#[from] types::Error), | |
| #[error("Decoding error: {0}")] | |
| DecodingError(String), | |
| #[error("Arg not found: {0}")] | |
| ArgNotFound(String), | |
| #[error("Arg type not found: {0}")] | |
| ArgTypeNotFound(String) | |
| } |
| let arg = Argument { | ||
| ty: Type(*inner.clone()), | ||
| ..arg.clone() | ||
| }; | ||
| flat_arg(&arg, types, true) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary cloning of arg in recursion
In the flat_arg function, the arg is cloned when creating a new Argument for list elements. To improve performance, consider borrowing or reusing arg where possible.
Apply this diff to prevent unnecessary cloning:
let arg = Argument {
ty: Type(*inner.clone()),
- ..arg.clone()
+ name: arg.name.clone(),
+ optional: arg.optional,
+ description: arg.description.clone(),
};
flat_arg(&arg, types, true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let arg = Argument { | |
| ty: Type(*inner.clone()), | |
| ..arg.clone() | |
| }; | |
| flat_arg(&arg, types, true) | |
| } | |
| let arg = Argument { | |
| ty: Type(*inner.clone()), | |
| name: arg.name.clone(), | |
| optional: arg.optional, | |
| description: arg.description.clone(), | |
| }; | |
| flat_arg(&arg, types, true) | |
| } |
odra-cli/src/types.rs
Outdated
| let bytes = parts | ||
| .iter() | ||
| .map(|part| parse_hex(input)) |
There was a problem hiding this comment.
Shouldn't this be .map(|part| parse_hex(part))?
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (4)
odra-cli/src/container.rs (2)
131-135: Handle potential errors when determining the project root path.In the
file_pathmethod, the call toproject_root::get_project_root()may fail, resulting in anIoerror. While the error is being propagated, providing more context could help with debugging.Consider enhancing the error handling to include more descriptive context:
fn file_path() -> Result<PathBuf, ContractError> { let mut path = project_root::get_project_root() - .map_err(ContractError::Io)?; + .map_err(|e| ContractError::Io(std::io::Error::new(e.kind(), format!("Failed to get project root: {}", e))))?; path.push(DEPLOYED_CONTRACTS_FILE); Ok(path) }This modification adds more information to the error message, which can assist in diagnosing issues related to path resolution.
139-152: DerivePartialEqandEqforDeployedContractfor better usability.Adding
PartialEqandEqderivations to theDeployedContractstruct can enhance its usability, especially if you need to compare contracts or check for duplicates.Apply this diff to derive
PartialEqandEq:#[derive(Deserialize, Serialize, Debug, Clone)] +#[derive(PartialEq, Eq)] struct DeployedContract { name: String, package_hash: String }This allows for straightforward comparisons of
DeployedContractinstances using==and!=.odra-cli/src/args.rs (2)
57-57: Nitpick: Enhancevalue_namefor better readabilityUsing
format!("{:?}", arg.ty)in.value_name()may produce verbose or less user-friendly type names in the command-line interface. Consider implementing a method to convertNamedCLTypeinto a more concise and readable string to improve the clarity of help messages displayed to the user.
358-358: Clarify the unit of measurement in help messageIn the help message for
attached_value_arg, the term "CSPRs" might not be immediately clear to all users. Consider clarifying the unit of measurement or providing a brief explanation to enhance user understanding.Apply this diff to improve the help message:
Arg::new(ARG_ATTACHED_VALUE) .help("The amount of CSPR (Casper tokens) attached to the call") .long(ARG_ATTACHED_VALUE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- odra-cli/src/args.rs (1 hunks)
- odra-cli/src/container.rs (1 hunks)
- odra-cli/src/types.rs (1 hunks)
🧰 Additional context used
| pub(crate) fn parse_value<T: FromStr>(value: &str) -> TypeResult<T> | ||
| where | ||
| <T as FromStr>::Err: Debug | ||
| { | ||
| <T as FromStr>::from_str(value).map_err(|_| Error::Parse(value.to_string())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include Original Error Details in parse_value Function
In the parse_value function, the original error from FromStr::from_str is discarded. Including the original error message can provide more context and aid in debugging when parsing fails.
Apply this diff to include the original error details:
- <T as FromStr>::from_str(value).map_err(|_| Error::Parse(value.to_string()))
+ <T as FromStr>::from_str(value).map_err(|e| Error::Parse(format!("Failed to parse '{}': {:?}", value, e)))This change captures the underlying error, making it easier to identify the cause of the parse failure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn parse_value<T: FromStr>(value: &str) -> TypeResult<T> | |
| where | |
| <T as FromStr>::Err: Debug | |
| { | |
| <T as FromStr>::from_str(value).map_err(|_| Error::Parse(value.to_string())) | |
| } | |
| pub(crate) fn parse_value<T: FromStr>(value: &str) -> TypeResult<T> | |
| where | |
| <T as FromStr>::Err: Debug | |
| { | |
| <T as FromStr>::from_str(value).map_err(|e| Error::Parse(format!("Failed to parse '{}': {:?}", value, e))) | |
| } |
| let mut hex = "0x".to_string(); | ||
| let mut dec = "".to_string(); | ||
| for val in input.iter().take(size) { | ||
| dec.push_str(&format!("{}, ", val)); | ||
| hex.push_str(&format!("{:02x}", val)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Byte Array String Construction
In the from_bytes function for NamedCLType::ByteArray, manual string manipulation is used to construct the hexadecimal and decimal representations of the byte array. Consider using iterator methods to simplify the code and improve readability.
Apply this diff:
- let mut hex = "0x".to_string();
- let mut dec = "".to_string();
- for val in input.iter().take(size) {
- dec.push_str(&format!("{}, ", val));
- hex.push_str(&format!("{:02x}", val));
- }
+ let hex = format!("0x{}", input.iter().take(size).map(|b| format!("{:02x}", b)).collect::<String>());
+ let dec = input.iter().take(size).map(|b| b.to_string()).collect::<Vec<_>>().join(", ");This refactor removes the need to manually remove trailing commas and enhances code clarity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut hex = "0x".to_string(); | |
| let mut dec = "".to_string(); | |
| for val in input.iter().take(size) { | |
| dec.push_str(&format!("{}, ", val)); | |
| hex.push_str(&format!("{:02x}", val)); | |
| } | |
| let hex = format!("0x{}", input.iter().take(size).map(|b| format!("{:02x}", b)).collect::<String>()); | |
| let dec = input.iter().take(size).map(|b| b.to_string()).collect::<Vec<_>>().join(", "); |
| NamedCLType::List(ty) => { | ||
| let (num_keys, mut stream) = from_bytes_or_err::<u32>(input)?; | ||
| let mut result = "".to_string(); | ||
| for _ in 0..num_keys { | ||
| let (v, rem) = from_bytes(ty, stream)?; | ||
| result.push_str(&v); | ||
| result.push(','); | ||
| stream = rem; | ||
| } | ||
| if num_keys > 0 { | ||
| result.pop(); | ||
| } | ||
| Ok((result, stream)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve String Construction in List Deserialization
When deserializing lists in the from_bytes function, manual string manipulation is used, which can be error-prone. Using a Vec to collect values and then joining them improves code readability and correctness.
Apply this diff:
- let mut result = "".to_string();
- for _ in 0..num_keys {
- let (v, rem) = from_bytes(ty, stream)?;
- result.push_str(&v);
- result.push(',');
- stream = rem;
- }
- if num_keys > 0 {
- result.pop();
- }
+ let mut values = Vec::new();
+ for _ in 0..num_keys {
+ let (v, rem) = from_bytes(ty, stream)?;
+ values.push(v);
+ stream = rem;
+ }
+ let result = values.join(",");This approach avoids manual string manipulation and potential off-by-one errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NamedCLType::List(ty) => { | |
| let (num_keys, mut stream) = from_bytes_or_err::<u32>(input)?; | |
| let mut result = "".to_string(); | |
| for _ in 0..num_keys { | |
| let (v, rem) = from_bytes(ty, stream)?; | |
| result.push_str(&v); | |
| result.push(','); | |
| stream = rem; | |
| } | |
| if num_keys > 0 { | |
| result.pop(); | |
| } | |
| Ok((result, stream)) | |
| } | |
| NamedCLType::List(ty) => { | |
| let (num_keys, mut stream) = from_bytes_or_err::<u32>(input)?; | |
| let mut values = Vec::new(); | |
| for _ in 0..num_keys { | |
| let (v, rem) = from_bytes(ty, stream)?; | |
| values.push(v); | |
| stream = rem; | |
| } | |
| let result = values.join(","); | |
| Ok((result, stream)) | |
| } |
| pub(crate) fn to_bytes_or_err<T: ToBytes>(input: T) -> TypeResult<Vec<u8>> { | ||
| input.to_bytes().map_err(|_| Error::Serialization) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve Underlying Error in to_bytes_or_err Function
Similarly, in the to_bytes_or_err function, the original error from ToBytes::to_bytes is discarded. Including the original error can help diagnose serialization issues.
Apply this diff:
- input.to_bytes().map_err(|_| Error::Serialization)
+ input.to_bytes().map_err(|e| Error::SerializationWithContext(format!("{:?}", e)))Update the Error enum accordingly:
+ #[error("Serialization error: {0}")]
+ SerializationWithContext(String),This change provides additional context for serialization errors.
Committable suggestion was skipped due to low confidence.
| NamedCLType::Option(ty) => { | ||
| if input.first() == Some(&OPTION_NONE_TAG) { | ||
| Ok(("null".to_string(), input)) | ||
| } else { | ||
| from_bytes(ty, &input[1..]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Advance Input Slice After Reading OPTION_NONE_TAG
In the from_bytes function's handling of NamedCLType::Option, the input slice is not advanced when the variant is OPTION_NONE_TAG. This could result in incorrect parsing of subsequent data.
Apply this diff to correctly advance the input slice:
- if input.first() == Some(&OPTION_NONE_TAG) {
- Ok(("null".to_string(), input))
+ if let Some((OPTION_NONE_TAG, rem)) = input.split_first() {
+ Ok(("null".to_string(), rem))This adjustment ensures that the input slice rem correctly excludes the consumed tag byte.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NamedCLType::Option(ty) => { | |
| if input.first() == Some(&OPTION_NONE_TAG) { | |
| Ok(("null".to_string(), input)) | |
| } else { | |
| from_bytes(ty, &input[1..]) | |
| } | |
| } | |
| NamedCLType::Option(ty) => { | |
| if let Some((OPTION_NONE_TAG, rem)) = input.split_first() { | |
| Ok(("null".to_string(), rem)) | |
| } else { | |
| from_bytes(ty, &input[1..]) | |
| } | |
| } |
| if input.is_empty() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Ensure required arguments are handled appropriately
In the compose function, when the input for an argument is empty, the code continues without inserting the argument into runtime_args. If the argument is required, this could lead to missing required arguments being silently ignored. Consider adding a check to verify that required arguments are provided, and return an ArgsError::ArgNotFound error if they are missing.
Apply this diff to handle missing required arguments:
if input.is_empty() {
+ if !arg.optional {
+ return Err(ArgsError::ArgNotFound(arg.name.clone()));
+ }
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if input.is_empty() { | |
| continue; | |
| } | |
| if input.is_empty() { | |
| if !arg.optional { | |
| return Err(ArgsError::ArgNotFound(arg.name.clone())); | |
| } | |
| continue; | |
| } |
| let parent = parts[parts.len() - 2].clone(); | ||
|
|
||
| if current_group.name != parent && is_list_element { |
There was a problem hiding this comment.
Prevent potential index out-of-bounds error
In build_complex_arg, accessing parts[parts.len() - 2] assumes that parts has at least two elements. If parts contains fewer than two elements, this will cause an index out-of-bounds error at runtime. Consider adding a check to ensure parts.len() >= 2 before accessing parts[parts.len() - 2], or adjust the logic to handle cases where the argument name does not contain a parent component.
Apply this diff to safely access the parent component:
let parent = if parts.len() >= 2 {
parts[parts.len() - 2].clone()
} else {
- parts[parts.len() - 2].clone()
+ "".to_string()
};Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
odra-cli/src/types.rs (1)
21-22: Remove commented-out codeThe constants
PREFIX_NONEandPREFIX_SOMEare commented out and appear to be unused. Removing unused code helps maintain a clean and readable codebase.core/src/host.rs (2)
413-421: Consider renaming variablebindingfor clarityThe variable
bindingrepresents a mutable reference toself.events_count. For better readability, consider renaming it to something more descriptive, such asevents_count_mutorevents_count_map.
415-420: Consider initializingevents_countduring contract registrationBy moving the initialization of
events_countfromregister_contracttoraw_call_contract, there might be cases whereevents_countis uninitialized before any contract calls. If other methods rely onevents_countbeforeraw_call_contractis invoked, this could lead to inconsistencies or errors. Consider initializingevents_countwhen registering the contract to ensure consistency across all operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- core/src/host.rs (1 hunks)
- odra-cli/src/types.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
odra-cli/src/types.rs (1)
118-123: 🛠️ Refactor suggestionInclude original error details in
parse_valuefunctionThe
parse_valuefunction discards the original error fromFromStr::from_str. Including the error details enhances debugging.Apply this diff:
122 <T as FromStr>::from_str(value).map_err(|e| Error::Parse(format!("Failed to parse '{}': {:?}", value, e)))Likely invalid or redundant comment.
core/src/host.rs (1)
415-420:⚠️ Potential issueAvoid using
unwrap(); handle potential errors frombackend.get_eventThe current code uses
unwrap()when callingbackend.get_event(contract_address, event_id), which will cause a panic ifget_eventreturns an error. To improve error handling and ensure robustness, consider handling the potential errors properly, either by propagating the error using the?operator or by implementing appropriate error-handling logic.Here's how you could modify the code:
let mut events = vec![]; for event_id in old_events_last_id..new_events_count { - let event = backend.get_event(contract_address, event_id).unwrap(); + let event = backend.get_event(contract_address, event_id)?; events.push(event); }Alternatively, explicitly handle the error:
let mut events = vec![]; for event_id in old_events_last_id..new_events_count { + match backend.get_event(contract_address, event_id) { + Ok(event) => events.push(event), + Err(e) => { + // Handle the error appropriately + return Err(OdraError::from(e)); + } + } }This ensures that any errors are properly handled rather than causing a panic.
Likely invalid or redundant comment.
| Err(e) => Err(e) | ||
| } | ||
| } | ||
| NamedCLType::Custom(_) => unreachable!("should not be here") |
There was a problem hiding this comment.
Avoid using unreachable! macro in production code
Using unreachable! at lines 310 and 417 can cause the program to panic if an unexpected variant is encountered. Instead, consider handling this case gracefully by returning an appropriate error. This approach enhances the robustness of your code.
Apply this diff to handle the unexpected variant:
310 NamedCLType::Custom(_) => {
- unreachable!("should not be here")
+ return Err(Error::UnsupportedType("Custom NamedCLType variant is not supported".to_string()))
}417 NamedCLType::Custom(_) => {
- unreachable!("should not be here")
+ return Err(Error::UnsupportedType("Custom NamedCLType variant is not supported".to_string()))
}You'll need to add the UnsupportedType variant to your Error enum:
88 #[error("Unexpected error: {0}")]
89 Other(String)
+90 #[error("Unsupported type: {0}")]
+91 UnsupportedType(String)Also applies to: 417-417
| pub(crate) fn from_bytes_or_err<T: FromBytes>(input: &[u8]) -> TypeResult<(T, &[u8])> { | ||
| T::from_bytes(input).map_err(|_| Error::Deserialization) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve underlying errors in serialization and deserialization
In from_bytes_or_err and to_bytes_or_err functions, the original errors are not conveyed. Including the original error messages can aid in debugging serialization issues.
For from_bytes_or_err:
429 pub(crate) fn from_bytes_or_err<T: FromBytes>(input: &[u8]) -> TypeResult<(T, &[u8])> {
430 T::from_bytes(input).map_err(|e| Error::Deserialization(format!("Failed to deserialize: {:?}", e)))
431 }For to_bytes_or_err:
434 pub(crate) fn to_bytes_or_err<T: ToBytes>(input: T) -> TypeResult<Vec<u8>> {
435 input.to_bytes().map_err(|e| Error::Serialization(format!("Failed to serialize: {:?}", e)))
436 }Update the Error enum to include context:
75 #[error("Serialization error")]
-76 Serialization,
+76 Serialization(String),
77 #[error("Deserialization error")]
-78 Deserialization,
+78 Deserialization(String),Also applies to: 434-436
| fn parse_hex(input: &str) -> TypeResult<Vec<u8>> { | ||
| match input.strip_prefix("0x") { | ||
| Some(data) => hex::decode(data).map_err(|_| Error::HexDecode), | ||
| None => Err(Error::InvalidHexString) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include original error details in parse_hex function
In the parse_hex function, the original error from hex::decode is discarded. Capturing the underlying error can provide valuable context for debugging.
Apply this diff to include the original error details:
421 fn parse_hex(input: &str) -> TypeResult<Vec<u8>> {
422 match input.strip_prefix("0x") {
423 Some(data) => hex::decode(data).map_err(|e| Error::HexDecode(format!("Failed to decode '{}': {:?}", data, e))),
424 None => Err(Error::InvalidHexString)
425 }Update the Error enum accordingly:
69 #[error("Hex decode error")]
-70 HexDecode,
+70 HexDecode(String),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
odra-cli/src/args.rs (1)
371-475: Expand unit tests to cover edge cases and error handlingThe current tests verify standard functionality but may not cover edge cases or invalid inputs. Consider adding tests for scenarios such as missing arguments, invalid types, or decoding errors to ensure robustness.
Would you like assistance in creating additional unit tests to cover these cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- odra-cli/src/args.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
odra-cli/src/args.rs (3)
163-165:⚠️ Potential issueEnsure required arguments are handled appropriately
In the
composefunction, when the input for an argument is empty, the code continues without inserting the argument intoruntime_args. If the argument is required, this could lead to missing required arguments being silently ignored. Consider adding a check to verify that required arguments are provided, and return anArgsError::ArgNotFounderror if they are missing.Apply this diff to handle missing required arguments:
if input.is_empty() { + if !arg.optional { + return Err(ArgsError::ArgNotFound(arg.name.clone())); + } continue; }Likely invalid or redundant comment.
260-262:⚠️ Potential issuePrevent potential index out-of-bounds error
In
build_complex_arg, accessingparts[parts.len() - 2]assumes thatpartshas at least two elements. Ifpartscontains fewer than two elements, this will cause an index out-of-bounds error at runtime. Consider adding a check to ensureparts.len() >= 2before accessingparts[parts.len() - 2], or adjust the logic to handle cases where the argument name does not contain a parent component.Apply this diff to safely access the parent component:
-let parent = parts[parts.len() - 2].clone(); +let parent = if parts.len() >= 2 { + parts[parts.len() - 2].clone() +} else { + "".to_string() +};Likely invalid or redundant comment.
313-317:⚠️ Potential issueHandle potential missing variants in enum decoding
In the
decodefunction, when decoding an enum variant, the code assumes that the discriminant will match a variant. It's good practice to handle the case where a matching variant is not found, perhaps by providing a more descriptive error.Consider updating the error handling as follows:
let variant = variants .iter() .find(|v| v.discriminant == discriminant) - .ok_or(ArgsError::DecodingError("Variant not found".to_string()))?; + .ok_or_else(|| ArgsError::DecodingError(format!("Variant with discriminant {} not found in {}", discriminant, name)))?;Likely invalid or redundant comment.
839ae31 to
200ece2
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
odra-cli/src/container.rs (1)
1-153: Overall, the implementation is solid with room for some improvements.The
DeployedContractsContainerprovides a robust solution for managing deployed smart contracts. The error handling, serialization/deserialization, and file I/O operations are well-implemented. However, there are a few areas that could be improved:
- Add a check for duplicate contracts in the
add_contractmethod.- Simplify the
get_refmethod for better readability.- Fix the file deletion logic in the
handle_previous_versionmethod.- Ensure the 'resources' directory exists before saving the file in the
save_atmethod.Addressing these points will enhance the robustness and maintainability of the code.
Consider adding unit tests to verify the behavior of the
DeployedContractsContainer, especially around file I/O operations and contract management. This will help ensure the reliability of the implementation as the project evolves.odra-cli/src/args.rs (1)
371-475: Expand unit tests to cover edge cases and error handlingThe current tests verify standard functionality but may not cover edge cases or invalid inputs. Consider adding tests for scenarios such as missing arguments, invalid types, or decoding errors to ensure robustness.
Would you like assistance in creating additional unit tests to cover these cases?
core/src/host.rs (2)
415-420: LGTM! Consider a minor optimization.The changes in the
raw_call_contractmethod improve the robustness of event tracking by ensuring that event counts are initialized for all deployed contracts. This is a good improvement that handles cases where contracts might have been registered without initializing their event counts.Consider using the
entrymethod of theBTreeMapto avoid the double lookup:-if binding.get(contract_address).is_none() { - binding.insert( - *contract_address, - backend.get_events_count(contract_address) - ); -} -let events_count = binding.get_mut(contract_address).unwrap(); +let events_count = binding.entry(*contract_address).or_insert_with(|| backend.get_events_count(contract_address));This change would slightly improve performance and make the code more concise.
Line range hint
1-724: LGTM! Consider updating documentation.The removal of event count initialization from the
register_contractmethod is consistent with the changes inraw_call_contract. This change makes the contract registration process lighter by deferring the event count retrieval to when it's actually needed.Consider updating the documentation for the
register_contractmethod to reflect that it no longer initializes the event count. This will help maintain accurate documentation and prevent confusion for future developers.odra-cli/src/cmd/deploy.rs (1)
32-33: Fix inconsistent indentation in documentation commentsThe documentation comments on lines 32-33 are indented inconsistently compared to the rest of the code. Please adjust the indentation to align with standard formatting practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- core/src/host.rs (1 hunks)
- odra-cli/src/args.rs (1 hunks)
- odra-cli/src/cmd/deploy.rs (1 hunks)
- odra-cli/src/cmd/scenario.rs (1 hunks)
- odra-cli/src/container.rs (1 hunks)
- odra-cli/src/test_utils.rs (1 hunks)
- odra-cli/src/types.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- odra-cli/src/types.rs
🧰 Additional context used
🔇 Additional comments (21)
odra-cli/src/container.rs (8)
1-13: LGTM: Imports and constant definition are appropriate.The imports cover all necessary modules and traits for the functionality of this file. The constant
DEPLOYED_CONTRACTS_FILEis well-defined and clearly indicates the location of the deployed contracts file.
15-25: LGTM: Well-defined error enum with clear messages.The
ContractErrorenum is well-structured usingthiserror, providing clear and informative error messages for each variant. It covers the expected error types for TOML operations, I/O, and contract not found scenarios.
27-38: LGTM: Well-structured container for deployed contracts.The
DeployedContractsContainerstruct is well-designed with appropriate fields for storing deployment time and a list of deployed contracts. The use of serde traits (Deserialize,Serialize) enables easy serialization and deserialization, which is crucial for storing and retrieving the data from a TOML file.
42-49: LGTM: Constructor method is well-implemented.The
newmethod correctly initializes a newDeployedContractsContainerwith the current timestamp and an empty vector of contracts. It also handles the previous version of the file, which is a good practice for maintaining data integrity.
77-82: LGTM: Theaddressmethod is well-implemented.The
addressmethod correctly finds and returns the address of a contract by its name. The use ofOptionis appropriate here, as the contract may not exist.
85-91: LGTM: Theloadmethod is correctly implemented.The
loadmethod properly reads the file, deserializes its content, and handles potential errors. The use of?operator for error propagation is a good practice.
121-123: LGTM: Helper methods are well-implemented.The
time,update, andfile_pathmethods are correctly implemented and provide necessary functionality for managing the deployed contracts file.Also applies to: 126-129, 131-136
139-153: LGTM: DeployedContract struct and implementation are well-designed.The
DeployedContractstruct correctly represents a deployed contract with its name and package hash. Thenewmethod provides a convenient way to create a new instance from a generic type that implementsHasIdentand anAddress. This design allows for easy creation and management of deployed contracts.odra-cli/src/args.rs (1)
130-135: 🛠️ Refactor suggestionOptimize performance by avoiding unnecessary cloning
In the
flat_argfunction, theargis unnecessarily cloned when creating a newArgumentfor list elements. To improve performance, consider borrowing or reusingargwhere possible.Apply this diff to prevent unnecessary cloning:
let arg = Argument { ty: Type(*inner.clone()), - ..arg.clone() + name: arg.name.clone(), + optional: arg.optional, + description: arg.description.clone(), }; flat_arg(&arg, types, true)Likely invalid or redundant comment.
core/src/host.rs (1)
Line range hint
1-724: Overall improvements in event tracking and contract registrationThe changes in this file contribute positively to the PR objectives:
- Event tracking is more robust, with lazy initialization of event counts.
- Contract registration is more efficient, deferring event count retrieval.
- These changes should contribute to improved performance of the CLI, especially when dealing with multiple contracts.
The code quality is good, with only minor suggestions for optimization and documentation updates. These changes align well with the goal of streamlining the project structure and addressing performance concerns.
odra-cli/src/cmd/deploy.rs (2)
23-28: Previous review comment is still validThe previous review comment about enhancing error handling in the
runmethod is still applicable.
43-49: Previous review comment is still validThe previous review comment regarding including the underlying
OdraErrorinDeployErrorto preserve error context is still valid.odra-cli/src/test_utils.rs (2)
9-30:mock_entry_pointfunction is well-implementedThe
mock_entry_pointfunction correctly constructs anEntrypointwith appropriate fields and argument definitions, including custom types and a list of U8 types.
32-60: Inconsistent type forsignatureargumentIn the
mock_command_argsfunction, thesignatureargument is defined with typeNamedCLType::U8. However, in themock_entry_pointfunction, thesignatureargument is defined as a list of unsigned bytes usingNamedCLType::List(Box::new(NamedCLType::U8)). This inconsistency may lead to type mismatches when processing thesignatureargument.odra-cli/src/cmd/scenario.rs (7)
14-29: Well-structured definition of theScenariotraitThe
Scenariotrait is clearly defined with essential methods for custom scenario implementations. Providing a default implementation forargsenhances usability.
34-37: Appropriate encapsulation inScenarioCmdstructThe
ScenarioCmdstruct effectively encapsulates scenario commands with a name and boxedScenario. The use ofpub(crate)visibility ensures controlled access within the crate.
39-46: Correct implementation ofScenarioCmd::newThe constructor correctly initializes a
ScenarioCmdinstance, capturing the scenario's name and boxing the scenario object. This approach promotes flexibility and type safety.
48-60: Effective execution flow inScenarioCmd::runThe
runmethod appropriately loads deployed contracts and processes command-line arguments before executing the scenario. The use of?operator for error propagation maintains clean code.
83-154: Robust argument handling inScenarioArgsThe
ScenarioArgsstruct and its methodsget_singleandget_manymanage scenario arguments effectively. The use of typed deserialization ensures that arguments are correctly interpreted.
156-167: Comprehensive error definitions inArgErrorThe
ArgErrorenum covers a range of possible argument parsing errors with clear and descriptive messages, aiding in debugging and user feedback.
174-178: Clear metadata representation inScenarioMetadatatraitThe
ScenarioMetadatatrait succinctly defines static metadata for scenarios, facilitating consistent naming and descriptions across different scenario implementations.
| pub fn add_contract<T: HostRef + HasIdent>( | ||
| &mut self, | ||
| contract: &T | ||
| ) -> Result<(), ContractError> { | ||
| self.contracts | ||
| .push(DeployedContract::new::<T>(contract.address())); | ||
| self.update() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a check for duplicate contracts.
The add_contract method currently doesn't check if a contract with the same name already exists in the container. This could lead to unexpected behavior if a contract is added multiple times.
Consider adding a check before pushing a new contract:
pub fn add_contract<T: HostRef + HasIdent>(
&mut self,
contract: &T
) -> Result<(), ContractError> {
let name = T::ident();
if self.contracts.iter().any(|c| c.name == name) {
return Err(ContractError::DuplicateContract(name));
}
self.contracts.push(DeployedContract::new::<T>(contract.address()));
self.update()
}You'll need to add a new variant to ContractError:
#[error("Contract `{0}` already exists")]
DuplicateContract(String),| pub fn get_ref<T: OdraContract + 'static>( | ||
| &self, | ||
| env: &HostEnv | ||
| ) -> Result<T::HostRef, ContractError> { | ||
| self.contracts | ||
| .iter() | ||
| .find(|c| c.name == T::HostRef::ident()) | ||
| .map(|c| Address::from_str(&c.package_hash).ok()) | ||
| .and_then(|opt| opt.map(|addr| <T as HostRefLoader<T::HostRef>>::load(env, addr))) | ||
| .ok_or(ContractError::NotFound(T::HostRef::ident())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the get_ref method for better readability.
The current implementation of get_ref uses chained map and and_then calls, which can be hard to read and maintain.
Consider refactoring the method as follows:
pub fn get_ref<T: OdraContract + 'static>(
&self,
env: &HostEnv
) -> Result<T::HostRef, ContractError> {
let contract = self.contracts
.iter()
.find(|c| c.name == T::HostRef::ident())
.ok_or_else(|| ContractError::NotFound(T::HostRef::ident()))?;
let address = Address::from_str(&contract.package_hash)
.map_err(|_| ContractError::NotFound(T::HostRef::ident()))?;
Ok(<T as HostRefLoader<T::HostRef>>::load(env, address))
}This refactoring separates each step, making the code more straightforward to understand and maintain.
| fn save_at(&self, file_path: &PathBuf) -> Result<(), ContractError> { | ||
| let content = toml::to_string_pretty(&self).map_err(ContractError::TomlSerialize)?; | ||
| let mut file = File::create(file_path).map_err(ContractError::Io)?; | ||
|
|
||
| file.write_all(content.as_bytes()) | ||
| .map_err(ContractError::Io)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the 'resources' directory exists before saving the file.
The save_at method assumes that the parent directory of the file exists. This may not always be the case, especially when running the program for the first time.
Consider creating the parent directory if it doesn't exist:
fn save_at(&self, file_path: &PathBuf) -> Result<(), ContractError> {
if let Some(parent) = file_path.parent() {
std::fs::create_dir_all(parent).map_err(ContractError::Io)?;
}
let content = toml::to_string_pretty(&self).map_err(ContractError::TomlSerialize)?;
let mut file = File::create(file_path).map_err(ContractError::Io)?;
file.write_all(content.as_bytes())
.map_err(ContractError::Io)?;
Ok(())
}This change ensures that the directory structure exists before attempting to create the file.
| pub(crate) fn handle_previous_version() -> Result<(), ContractError> { | ||
| if let Ok(deployed_contracts) = Self::load() { | ||
| // Build new file name. | ||
| let date = deployed_contracts.time(); | ||
| let mut path = project_root::get_project_root().map_err(ContractError::Io)?; | ||
| path.push(format!("{}.{}", DEPLOYED_CONTRACTS_FILE, date)); | ||
|
|
||
| // Store previous version under new file name. | ||
| deployed_contracts.save_at(&path)?; | ||
|
|
||
| // Remove old file. | ||
| std::fs::remove_file(path).map_err(ContractError::Io)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Fix the file deletion logic in handle_previous_version.
The current implementation deletes the newly created backup file instead of the original file. This defeats the purpose of creating a backup.
Apply this diff to correct the file deletion:
pub(crate) fn handle_previous_version() -> Result<(), ContractError> {
if let Ok(deployed_contracts) = Self::load() {
// Build new file name.
let date = deployed_contracts.time();
let mut path = project_root::get_project_root().map_err(ContractError::Io)?;
path.push(format!("{}.{}", DEPLOYED_CONTRACTS_FILE, date));
// Store previous version under new file name.
deployed_contracts.save_at(&path)?;
// Remove old file.
- std::fs::remove_file(path).map_err(ContractError::Io)?;
+ std::fs::remove_file(Self::file_path()?).map_err(ContractError::Io)?;
}
Ok(())
}This change ensures that the original file is deleted after creating the backup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn handle_previous_version() -> Result<(), ContractError> { | |
| if let Ok(deployed_contracts) = Self::load() { | |
| // Build new file name. | |
| let date = deployed_contracts.time(); | |
| let mut path = project_root::get_project_root().map_err(ContractError::Io)?; | |
| path.push(format!("{}.{}", DEPLOYED_CONTRACTS_FILE, date)); | |
| // Store previous version under new file name. | |
| deployed_contracts.save_at(&path)?; | |
| // Remove old file. | |
| std::fs::remove_file(path).map_err(ContractError::Io)?; | |
| } | |
| Ok(()) | |
| } | |
| pub(crate) fn handle_previous_version() -> Result<(), ContractError> { | |
| if let Ok(deployed_contracts) = Self::load() { | |
| // Build new file name. | |
| let date = deployed_contracts.time(); | |
| let mut path = project_root::get_project_root().map_err(ContractError::Io)?; | |
| path.push(format!("{}.{}", DEPLOYED_CONTRACTS_FILE, date)); | |
| // Store previous version under new file name. | |
| deployed_contracts.save_at(&path)?; | |
| // Remove old file. | |
| std::fs::remove_file(Self::file_path()?).map_err(ContractError::Io)?; | |
| } | |
| Ok(()) | |
| } |
|
|
||
| let variant = variants | ||
| .iter() | ||
| .find(|v| v.discriminant == discriminant) | ||
| .ok_or(ArgsError::DecodingError("Variant not found".to_string()))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for enum variant decoding
In the decode function, when decoding an enum variant, the code assumes that the discriminant will match a variant. It's good practice to handle the case where a matching variant is not found, perhaps by providing a more descriptive error.
Consider updating the error handling as follows:
let variant = variants
.iter()
.find(|v| v.discriminant == discriminant)
- .ok_or(ArgsError::DecodingError("Variant not found".to_string()))?;
+ .ok_or_else(|| ArgsError::DecodingError(format!("Variant with discriminant {} not found in {}", discriminant, name)))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let variant = variants | |
| .iter() | |
| .find(|v| v.discriminant == discriminant) | |
| .ok_or(ArgsError::DecodingError("Variant not found".to_string()))?; | |
| let variant = variants | |
| .iter() | |
| .find(|v| v.discriminant == discriminant) | |
| .ok_or_else(|| ArgsError::DecodingError(format!("Variant with discriminant {} not found in {}", discriminant, name)))?; |
| fn build_complex_arg(args: Vec<CommandArg>, matches: &ArgMatches) -> Result<CLValue, ArgsError> { | ||
| let mut current_group = ComposedArg::new(""); | ||
| let mut buffer: Vec<u8> = vec![]; | ||
| for arg in args { | ||
| let args = matches | ||
| .get_many::<String>(&arg.name) | ||
| .ok_or(ArgsError::ArgNotFound(arg.name.clone()))? | ||
| .map(|v| v.as_str()) | ||
| .collect::<Vec<_>>(); | ||
| let ty = arg.ty; | ||
| let is_list_element = arg.is_list_element; | ||
|
|
||
| let parts = arg | ||
| .name | ||
| .split('.') | ||
| .map(|s| s.to_string()) | ||
| .collect::<Vec<_>>(); | ||
| let parent = parts[parts.len() - 2].clone(); | ||
|
|
||
| if current_group.name != parent && is_list_element { | ||
| current_group.flush(&mut buffer)?; | ||
| current_group = ComposedArg::new(&parent); | ||
| current_group.add((ty, args)); | ||
| } else if current_group.name == parent && is_list_element { | ||
| current_group.add((ty, args)); | ||
| } else { | ||
| current_group.flush(&mut buffer)?; | ||
| let bytes = types::into_bytes(&ty, args[0])?; | ||
| buffer.extend_from_slice(&bytes); | ||
| } | ||
| } | ||
| current_group.flush(&mut buffer)?; | ||
| Ok(CLValue::from_components(CLType::Any, buffer)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize ComposedArg with the correct initial group name
In the build_complex_arg function, the initial current_group is created with an empty name, which may cause logical issues when comparing group names. Initialize it with the parent name of the first argument to ensure correct grouping.
Apply this diff to initialize current_group properly:
fn build_complex_arg(args: Vec<CommandArg>, matches: &ArgMatches) -> Result<CLValue, ArgsError> {
- let mut current_group = ComposedArg::new("");
+ let initial_parent = args
+ .first()
+ .and_then(|arg| arg.name.rsplit('.').nth(1))
+ .unwrap_or("");
+ let mut current_group = ComposedArg::new(initial_parent);
let mut buffer: Vec<u8> = vec![];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn build_complex_arg(args: Vec<CommandArg>, matches: &ArgMatches) -> Result<CLValue, ArgsError> { | |
| let mut current_group = ComposedArg::new(""); | |
| let mut buffer: Vec<u8> = vec![]; | |
| for arg in args { | |
| let args = matches | |
| .get_many::<String>(&arg.name) | |
| .ok_or(ArgsError::ArgNotFound(arg.name.clone()))? | |
| .map(|v| v.as_str()) | |
| .collect::<Vec<_>>(); | |
| let ty = arg.ty; | |
| let is_list_element = arg.is_list_element; | |
| let parts = arg | |
| .name | |
| .split('.') | |
| .map(|s| s.to_string()) | |
| .collect::<Vec<_>>(); | |
| let parent = parts[parts.len() - 2].clone(); | |
| if current_group.name != parent && is_list_element { | |
| current_group.flush(&mut buffer)?; | |
| current_group = ComposedArg::new(&parent); | |
| current_group.add((ty, args)); | |
| } else if current_group.name == parent && is_list_element { | |
| current_group.add((ty, args)); | |
| } else { | |
| current_group.flush(&mut buffer)?; | |
| let bytes = types::into_bytes(&ty, args[0])?; | |
| buffer.extend_from_slice(&bytes); | |
| } | |
| } | |
| current_group.flush(&mut buffer)?; | |
| Ok(CLValue::from_components(CLType::Any, buffer)) | |
| fn build_complex_arg(args: Vec<CommandArg>, matches: &ArgMatches) -> Result<CLValue, ArgsError> { | |
| let initial_parent = args | |
| .first() | |
| .and_then(|arg| arg.name.rsplit('.').nth(1)) | |
| .unwrap_or(""); | |
| let mut current_group = ComposedArg::new(initial_parent); | |
| let mut buffer: Vec<u8> = vec![]; | |
| for arg in args { | |
| let args = matches | |
| .get_many::<String>(&arg.name) | |
| .ok_or(ArgsError::ArgNotFound(arg.name.clone()))? | |
| .map(|v| v.as_str()) | |
| .collect::<Vec<_>>(); | |
| let ty = arg.ty; | |
| let is_list_element = arg.is_list_element; | |
| let parts = arg | |
| .name | |
| .split('.') | |
| .map(|s| s.to_string()) | |
| .collect::<Vec<_>>(); | |
| let parent = parts[parts.len() - 2].clone(); | |
| if current_group.name != parent && is_list_element { | |
| current_group.flush(&mut buffer)?; | |
| current_group = ComposedArg::new(&parent); | |
| current_group.add((ty, args)); | |
| } else if current_group.name == parent && is_list_element { | |
| current_group.add((ty, args)); | |
| } else { | |
| current_group.flush(&mut buffer)?; | |
| let bytes = types::into_bytes(&ty, args[0])?; | |
| buffer.extend_from_slice(&bytes); | |
| } | |
| } | |
| current_group.flush(&mut buffer)?; | |
| Ok(CLValue::from_components(CLType::Any, buffer)) |
| CommandArg::new("voucher.payment.buyer", "", NamedCLType::Key, true, false), | ||
| CommandArg::new( | ||
| "voucher.payment.payment_id", | ||
| "", | ||
| NamedCLType::String, | ||
| true, | ||
| false | ||
| ), | ||
| CommandArg::new("voucher.payment.amount", "", NamedCLType::U512, true, false), | ||
| CommandArg::new("voucher.names.label", "", NamedCLType::String, true, true), | ||
| CommandArg::new("voucher.names.owner", "", NamedCLType::Key, true, true), | ||
| CommandArg::new( | ||
| "voucher.names.token_expiration", | ||
| "", | ||
| NamedCLType::U64, | ||
| true, | ||
| true | ||
| ), | ||
| CommandArg::new( | ||
| "voucher.voucher_expiration", | ||
| "", | ||
| NamedCLType::U64, | ||
| true, | ||
| false | ||
| ), | ||
| CommandArg::new("signature", "", NamedCLType::U8, true, true), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor mock_command_args to reduce code duplication
The repeated use of CommandArg::new with similar parameters can be streamlined to enhance readability and maintainability. Consider creating a helper function to reduce code duplication.
Here's an example of how you might refactor the function:
fn create_command_arg(
name: &str,
cl_type: NamedCLType,
required: bool,
default: bool,
) -> CommandArg {
CommandArg::new(name, "", cl_type, required, default)
}
pub fn mock_command_args() -> Vec<CommandArg> {
vec![
create_command_arg("voucher.payment.buyer", NamedCLType::Key, true, false),
create_command_arg("voucher.payment.payment_id", NamedCLType::String, true, false),
create_command_arg("voucher.payment.amount", NamedCLType::U512, true, false),
create_command_arg("voucher.names.label", NamedCLType::String, true, true),
create_command_arg("voucher.names.owner", NamedCLType::Key, true, true),
create_command_arg("voucher.names.token_expiration", NamedCLType::U64, true, true),
create_command_arg("voucher.voucher_expiration", NamedCLType::U64, true, false),
create_command_arg("signature", NamedCLType::U8, true, true),
]
}| impl NameMintInfo { | ||
| pub fn new(label: &str, owner: &str, token_expiration: u64) -> Self { | ||
| Self { | ||
| label: label.to_string(), | ||
| owner: owner.parse().unwrap(), | ||
| token_expiration | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential parsing errors in NameMintInfo::new
The use of unwrap() on owner.parse() can lead to panics if parsing fails. Consider handling the error explicitly to enhance the robustness of the code.
Apply this diff to handle the error properly:
-impl NameMintInfo {
- pub fn new(label: &str, owner: &str, token_expiration: u64) -> Self {
- Self {
- label: label.to_string(),
- owner: owner.parse().unwrap(),
- token_expiration
- }
- }
+use std::error::Error;
+
+impl NameMintInfo {
+ pub fn new(label: &str, owner: &str, token_expiration: u64) -> Result<Self, Box<dyn Error>> {
+ Ok(Self {
+ label: label.to_string(),
+ owner: owner.parse()?,
+ token_expiration
+ })
+ }
}Adjust the function signature to return Result<Self, Box<dyn Error>> and handle the Result in the calling code to prevent potential panics.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl NameMintInfo { | |
| pub fn new(label: &str, owner: &str, token_expiration: u64) -> Self { | |
| Self { | |
| label: label.to_string(), | |
| owner: owner.parse().unwrap(), | |
| token_expiration | |
| } | |
| } | |
| } | |
| use std::error::Error; | |
| impl NameMintInfo { | |
| pub fn new(label: &str, owner: &str, token_expiration: u64) -> Result<Self, Box<dyn Error>> { | |
| Ok(Self { | |
| label: label.to_string(), | |
| owner: owner.parse()?, | |
| token_expiration | |
| }) | |
| } | |
| } |
| pub fn new(buyer: &str, payment_id: &str, amount: &str) -> Self { | ||
| Self { | ||
| buyer: buyer.parse().unwrap(), | ||
| payment_id: payment_id.to_string(), | ||
| amount: U512::from_dec_str(amount).unwrap() | ||
| } |
There was a problem hiding this comment.
Handle potential parsing errors in PaymentInfo::new
Using unwrap() on buyer.parse() and U512::from_dec_str(amount) can cause the program to panic if parsing fails. It's better to handle these errors explicitly to make the code more robust and prevent unexpected crashes.
Apply this diff to handle the errors properly:
-impl PaymentInfo {
- pub fn new(buyer: &str, payment_id: &str, amount: &str) -> Self {
- Self {
- buyer: buyer.parse().unwrap(),
- payment_id: payment_id.to_string(),
- amount: U512::from_dec_str(amount).unwrap()
- }
- }
+use std::error::Error;
+
+impl PaymentInfo {
+ pub fn new(buyer: &str, payment_id: &str, amount: &str) -> Result<Self, Box<dyn Error>> {
+ Ok(Self {
+ buyer: buyer.parse()?,
+ payment_id: payment_id.to_string(),
+ amount: U512::from_dec_str(amount)?
+ })
+ }
}You'll need to adjust the function signature to return Result<Self, Box<dyn Error>> and handle the Result accordingly wherever this constructor is called.
Committable suggestion was skipped due to low confidence.
Resolves #513
Summary by CodeRabbit
Summary by CodeRabbit
New Features
odra-clipackage for enhanced command-line interaction with Odra smart contracts.Bug Fixes
Documentation
Tests