Made container in cli scenarios mutable, so it is possible to deploy new #621
Conversation
WalkthroughRefactors DeployedContractsContainer to use interior mutability (RefCell), changes add/apply methods to take &self and return Result, and adds a ContractExists error; CLI change is documentation-only (a clarifying comment before the contract registration loop), no functional CLI behavior changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as odra-cli (cli)
participant Container as DeployedContractsContainer
participant Storage as ContractStorage
Note over CLI,Container: Registration flow (commented behavior)
CLI->>Container: query deployed contracts
loop per deployed contract
CLI->>CLI: lookup caller (commented expectation)
alt caller exists
CLI->>Container: add_contract_named(contract, Some(pkg))
Container->>Container: data.borrow_mut().add_contract(...)
Container->>Storage: storage.borrow_mut().write(updated)
Container-->>CLI: Result::Ok
else no caller
CLI-->>CLI: (skipped — noted in comment)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchmark report
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR enables mutable container access in CLI scenarios but introduces critical breaking changes that will fail existing template scenarios and cause compilation errors in dependent modules.
📄 Documentation Diagram
This diagram documents the refactored CLI command execution flow with mutable container support.
sequenceDiagram
participant User as User
participant CLI as CLI
participant Command as OdraCommand
participant Container as DeployedContractsContainer
User->>CLI: Execute command
CLI->>Command: run(env, args, types, &mut container)
note over Command: PR #35;621: Container is now mutable
Command->>Container: Access and modify contracts
Container-->>Command: Result
Command-->>CLI: Ok(())
CLI-->>User: Command executed successfully
🌟 Strengths
- Enables deployment of new contracts in CLI scenarios, enhancing flexibility for contract management.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | odra-cli/src/cmd/mod.rs | Architecture | Breaks template scenarios with immutable references | templates/full/bin/cli.rs |
| P1 | odra-cli/src/cmd/contract.rs | Architecture | Causes compilation failure due to signature mismatch | search:entry_point::call |
| P2 | odra-cli/src/cmd/mod.rs | Maintainability | Unnecessary code duplication in command traits | |
| P2 | odra-cli/src/cmd/events.rs | Architecture | Mutability over-applied to read-only operations | |
| P2 | odra-cli/src/cmd/whoami.rs | Maintainability | Forces mutability on side-effect free commands |
🔍 Notable Themes
- Mutability is inconsistently applied across commands, with read-only operations unnecessarily requiring mutable access, weakening API contracts and increasing risk of accidental mutations.
📈 Risk Diagram
This diagram illustrates the breaking change risks for existing Scenario implementations and entry_point calls.
sequenceDiagram
participant Scenario as Scenario Implementation
participant CLI as CLI
participant EntryPoint as entry_point::call
Scenario->>CLI: run(env, &container, args)
note over Scenario,CLI: R1(P1): Existing scenarios fail due to immutable signature
CLI--xScenario: Compilation error
CLI->>EntryPoint: call(env, ..., &mut container)
note over EntryPoint: R2(P1): Signature mismatch causes failure
EntryPoint--xCLI: Type error
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
odra-cli/src/container.rs (1)
161-169: Consider refactoring for clearer borrow management.The code correctly uses variable shadowing (line 166 shadows line 162) to drop the first mutable borrow of
storagebefore creating a second one. However, this pattern may not be immediately obvious to readers.Consider using explicit scoping to make the borrow lifetimes clearer:
DEPLOY_MODE_ARCHIVE => { - let storage = self.storage.borrow_mut(); - storage.backup()?; + self.storage.borrow_mut().backup()?; self.data.borrow_mut().contracts.clear(); let data = self.data.borrow(); let mut storage = self.storage.borrow_mut(); storage.write(&data)?; log("Starting fresh deployment. Previous contracts configuration has been backed up."); }This eliminates the shadowing by having the first borrow drop immediately after
backup().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odra-cli/src/cli.rs(2 hunks)odra-cli/src/container.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- odra-cli/src/cli.rs
🧰 Additional context used
🧬 Code graph analysis (1)
odra-cli/src/container.rs (2)
core/src/host.rs (5)
new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)odra-cli/src/utils.rs (1)
log(11-13)
🔇 Additional comments (4)
odra-cli/src/container.rs (4)
32-33: LGTM! Error variant added for duplicate contract detection.The new
ContractExistserror variant appropriately handles the case when attempting to add a contract with a duplicate package name.
176-195: LGTM! Borrow patterns are safe.The refactored methods correctly use interior mutability with
RefCell. The borrow sequence is safe:
- Mutable borrow for adding contract (dropped immediately)
- Immutable borrow for reading data
- Mutable borrow of storage for writing
The error propagation from the duplicate check is properly handled.
212-233: LGTM! ContractProvider implementation correctly uses immutable borrows.All trait methods properly use
.borrow()for read-only access to the interior data, which is the correct pattern for theRefCell-based design.
133-134: RefCell usage is appropriate and safe for this single-threaded CLI context.The
odra-clicrate is a standard single-threaded CLI application with no async or tokio runtime. Verification shows:
- No threading patterns in odra-cli (tokio/async only appear in lower-level client libraries, not the CLI)
- Borrow patterns in
apply_deploy_modeand other methods are properly scoped- Standard clap-based CLI architecture with linear command execution
RefCellis the correct pattern for interior mutability when&selfmethods need to mutate stateThe concern is resolved—the single-threaded guarantee is inherent to the CLI's architecture.
kubaplas
force-pushed
the
feature/mutable-container-in-cli
branch
from
9e26f8a to
98c258e
Compare
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
odra-cli/src/container.rs (1)
284-305: Fix misleading error message in duplicate check.The duplicate check correctly identifies when a contract with the same
package_namealready exists. However, the error message returns the new contract'spackage_hash, not the existing contract's identifier. This is confusing because the new contract doesn't exist yet, and the error message format "Contract{0}already exists" suggests it should reference the existing contract's name.Apply this diff to make the error message clearer:
// Check if a contract with this package_name already exists if self .contracts .iter() .any(|c| c.package_name == contract.package_name) { return Err(ContractError::ContractExists( - contract.package_hash.to_string() + contract.package_name.to_string() )); }This way, the error message will read:
Contract 'my_contract_name' already exists, which accurately reflects that a contract with that name already exists.
🧹 Nitpick comments (2)
odra-cli/src/cli.rs (1)
160-172: Consider logging skipped contracts for debugging.The registration now silently skips contracts without corresponding callers, which is appropriate for the new design. However, adding optional debug logging for skipped contracts could help diagnose configuration issues during development.
If helpful, you could add logging like:
for deployed_contract in container.all_contracts() { if let Some(caller) = self .callers .get(&(deployed_contract.name(), deployed_contract.key_name())) { self.host_env.register_contract( deployed_contract.address(), deployed_contract.key_name(), caller.clone() ); + } else { + // Optional: log for debugging + // log::debug!("Skipping contract {} - no caller registered", deployed_contract.name()); } }odra-cli/src/container.rs (1)
152-173: Consider explicit scoping for RefCell borrows.The multiple
borrow_mut()calls work correctly due to Rust's non-lexical lifetimes, but explicit scoping could improve readability and make the borrow boundaries clearer.For example, in the ARCHIVE mode:
DEPLOY_MODE_ARCHIVE => { - let storage = self.storage.borrow_mut(); - storage.backup()?; + { + let storage = self.storage.borrow(); + storage.backup()?; + } self.data.borrow_mut().contracts.clear(); let data = self.data.borrow(); let mut storage = self.storage.borrow_mut(); storage.write(&data)?;Note:
backup()should also take&selfinstead of&mut selfif it doesn't modify the storage state, but that's outside this file's scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odra-cli/src/cli.rs(2 hunks)odra-cli/src/container.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
odra-cli/src/container.rs (2)
core/src/host.rs (5)
new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)odra-cli/src/utils.rs (1)
log(11-13)
🔇 Additional comments (4)
odra-cli/src/cli.rs (1)
22-22: Import simplified appropriately.Removing
DEPLOYED_CONTRACTS_FILEfrom the public import aligns with the container's new interior mutability design, where storage details are encapsulated within the container.odra-cli/src/container.rs (3)
31-33: New error variant added appropriately.The
ContractExistsvariant correctly extends the error enum to handle duplicate contract scenarios introduced by the uniqueness check.
133-134: Interior mutability pattern implemented correctly.Using
RefCellto wrapdataandstorageenables mutation through&selfmethods, which is appropriate for this container's usage pattern in the CLI.
176-195: Contract addition with proper error propagation.The pattern of attempting to add the contract (which may fail with
ContractExists) and then persisting to storage only on success maintains consistency and prevents partial updates.
Benchmark report
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/2.4.1 #621 +/- ##
=================================================
+ Coverage 77.11% 78.42% +1.31%
=================================================
Files 204 217 +13
Lines 25894 28477 +2583
=================================================
+ Hits 19968 22333 +2365
- Misses 5926 6144 +218 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kubaplas
force-pushed
the
feature/mutable-container-in-cli
branch
from
98c258e to
4cf4781
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
odra-cli/src/cli.rs (1)
160-172: Add warning log when skipping contract registration.The silent skip behavior could hide configuration issues. Users may not realize why a deployed contract isn't responding to CLI commands if no corresponding caller was added during builder setup.
Consider adding a warning when skipping:
// Register the contracts from the container in the host environment. // Only register contracts that have callers (were added via .contract::<T>() in the builder). for deployed_contract in container.all_contracts() { if let Some(caller) = self .callers .get(&(deployed_contract.name(), deployed_contract.key_name())) { self.host_env.register_contract( deployed_contract.address(), deployed_contract.key_name(), caller.clone() ); + } else { + prettycli::warn(&format!( + "Skipping registration for contract '{}' - no caller found. Did you forget to call .contract::<T>() or .named_contract::<T>()?", + deployed_contract.key_name() + )); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odra-cli/src/cli.rs(2 hunks)odra-cli/src/container.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
odra-cli/src/container.rs (3)
core/src/host.rs (7)
new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)address(45-47)address(1038-1040)odra-cli/src/utils.rs (1)
log(11-13)core/src/address.rs (2)
address(36-36)address(40-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: Evaluate benchmark
- GitHub Check: Calculate test coverage
- GitHub Check: Benchmark
🔇 Additional comments (7)
odra-cli/src/cli.rs (1)
22-22: LGTM!The import cleanup correctly removes the unused
DEPLOYED_CONTRACTS_FILEconstant sinceFileContractStoragenow handles the path internally.odra-cli/src/container.rs (6)
31-33: LGTM!The new error variants improve error handling.
ContractExistsis properly used in the duplicate detection logic, andSchemaFileNotFoundprovides better error specificity for missing schema files.
133-134: LGTM - Interior mutability pattern appropriately applied.The RefCell-wrapped fields enable the container to be modified through shared references, which aligns with the PR objective to make the container mutable in CLI scenarios. The initialization properly wraps both success and error paths.
Also applies to: 142-143, 146-147
152-173: LGTM!The method correctly uses interior mutability to modify the container through a shared reference. The borrow patterns are safe - mutable borrows are dropped before subsequent borrows, and borrows on different RefCells don't conflict.
176-195: LGTM!The methods correctly implement interior mutability, and the signature change from
&mut selfto&selfenables the container to be shared more flexibly. Error propagation from bothadd_contract(duplicate detection) andwriteoperations is properly handled.
213-213: LGTM!The ContractProvider implementation correctly uses immutable borrows to access data through the RefCell. All borrows are short-lived and used only for read operations.
Also applies to: 223-223, 228-228
284-305: LGTM - Duplicate detection correctly implemented.The duplicate check properly validates uniqueness by
package_nameand returns a clear error message including the conflicting contract name. This addresses the concern from the past review comment, which has now been resolved - the error correctly returnspackage_nameinstead ofpackage_hash.
Benchmark report
|
kubaplas
force-pushed
the
feature/mutable-container-in-cli
branch
from
4cf4781 to
4e2423a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
odra-cli/src/container.rs (1)
152-173: Fix RefCell double-borrow inDEPLOY_MODE_ARCHIVEbranchIn
apply_deploy_mode, theDEPLOY_MODE_ARCHIVEarm borrowsself.storagemutably twice without dropping the first borrow, which will panic at runtime (already borrowed: BorrowMutError):let storage = self.storage.borrow_mut(); storage.backup()?; // `storage` (RefMut) is still in scope here ... let mut storage = self.storage.borrow_mut(); // second borrow_mut → panicThis is a critical correctness issue. Scope or explicitly drop the first
RefMutbefore borrowing again. For example:pub fn apply_deploy_mode(&self, mode: String) -> Result<(), ContractError> { match mode.as_str() { DEPLOY_MODE_OVERRIDE => { self.data.borrow_mut().contracts.clear(); let data = self.data.borrow(); - let mut storage = self.storage.borrow_mut(); - storage.write(&data)?; + self.storage.borrow_mut().write(&data)?; log("Contracts configuration has been overridden"); } DEPLOY_MODE_ARCHIVE => { - let storage = self.storage.borrow_mut(); - storage.backup()?; + { + let mut storage = self.storage.borrow_mut(); + storage.backup()?; + } // drop first RefMut before further borrows + self.data.borrow_mut().contracts.clear(); let data = self.data.borrow(); - let mut storage = self.storage.borrow_mut(); - storage.write(&data)?; + self.storage.borrow_mut().write(&data)?; log("Starting fresh deployment. Previous contracts configuration has been backed up."); } _ => {} // default mode does nothing } Ok(()) }Optionally, you may also want to treat a missing file on
backup()(first deploy) as a non-fatal condition rather than bubbling anIoerror, but that’s a behavior choice.
🧹 Nitpick comments (3)
odra-cli/src/cli.rs (1)
160-172: Registration guard is correct; tweak comment and consider visibility of skipped contractsThe new
if let Some(caller)guard correctly stops the CLI from failing when there is no corresponding caller for a deployed contract and only registers contracts that were actually added via the builder. Two small follow‑ups:
- The comment currently mentions only
.contract::<T>(), but this logic also covers.named_contract::<T>(). A more accurate wording would be “added via.contract::<T>()or.named_contract::<T>()in the builder.”- Optional: consider logging at debug/info level when a deployed contract is skipped (no entry in
self.callers), to aid debugging mismatches between the TOML file and the CLI configuration.- // Only register contracts that have callers (were added via .contract::<T>() in the builder). + // Only register contracts that have callers (were added via .contract::<T>() or + // .named_contract::<T>() in the builder). + + // Optionally, you may want to log skipped contracts here for easier debugging.odra-cli/src/container.rs (2)
130-150: Interior mutability onDeployedContractsContaineraligns with shared usageWrapping
ContractsDataandBox<dyn ContractStorage>inRefCellcleanly enables mutation through&self, which matches how the CLI now passes around&DeployedContractsContainer. Theinstanceconstructor correctly initializesdatafromstorage.read()when possible and falls back toDefaultotherwise.You might consider logging the
Err(_)fromstorage.read()before defaulting (e.g., bad TOML or missing file) so users can see why previous data was ignored, but that’s optional.- pub(crate) fn instance(storage: impl ContractStorage + 'static) -> Self { - match storage.read() { - Ok(data) => Self { + pub(crate) fn instance(storage: impl ContractStorage + 'static) -> Self { + match storage.read() { + Ok(data) => Self { data: std::cell::RefCell::new(data), storage: std::cell::RefCell::new(Box::new(storage)) }, - Err(_) => Self { + Err(e) => { + log(format!( + "Failed to read deployed contracts data, starting fresh: {e}" + )); + Self { data: std::cell::RefCell::new(Default::default()), storage: std::cell::RefCell::new(Box::new(storage)) - } + } + } } }
206-220: Consider returning a more informativeNotFounderror incontract_ref_namedThe lookup logic with
self.data.borrow().contracts()is fine and idiomatic withRefCell. One small improvement:
- When
package_nameisSome(name), the error currently returnsContractError::NotFound(T::HostRef::ident()), which hides the actual name the caller asked for.- Exposing the attempted
name(or at least including it in the error) would make debugging configuration mistakes easier.fn contract_ref_named<T: OdraContract + 'static>( &self, env: &HostEnv, package_name: Option<String> ) -> Result<T::HostRef, ContractError> { - let name = package_name.unwrap_or(T::HostRef::ident()); + let name = package_name.unwrap_or(T::HostRef::ident()); self.data .borrow() .contracts() .iter() .find(|c| c.package_name == name) .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())) + .ok_or(ContractError::NotFound(name)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odra-cli/src/cli.rs(2 hunks)odra-cli/src/container.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
odra-cli/src/container.rs (2)
core/src/host.rs (5)
new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)odra-cli/src/utils.rs (1)
log(11-13)
odra-cli/src/cli.rs (2)
examples/bin/odra_cli.rs (1)
container(50-50)odra-cli/src/utils.rs (2)
container(28-28)container(54-54)
🔇 Additional comments (5)
odra-cli/src/cli.rs (1)
16-25: Import narrowing toFileContractStoragelooks correct
DEPLOYED_CONTRACTS_FILEis no longer used directly in this file, so importing onlycontainer::FileContractStoragekeeps the surface clean and matches the new storage abstraction.odra-cli/src/container.rs (4)
20-34: Error variants for schema and duplicates are well-shapedAdding
SchemaFileNotFound(String)andContractExists(String)gives clear, typed errors for missing schemas and duplicate contract names. The messages are explicit and the payload type (String) is appropriate for both.
176-195:add_contract_named/add_contractcorrectly enforce uniqueness and persistThe refactor to take
&selfand useRefCellis sound:
ContractsData::add_contractis invoked under a singleborrow_mut, enforcing uniqueness and returningContractExistson duplicates.- On success, you re-borrow
dataimmutably andstoragemutably to write out the updated TOML, nicely encapsulating persistence.add_contractsimply delegates toadd_contract_namedwithNone, which keeps the API tidy.This matches the PR goal of interior mutability and makes duplicate handling explicit.
222-233: Read-side RefCell usage inall_contractsandaddress_by_nameis straightforwardBoth methods use
self.data.borrow()to access contracts and either clone the full list or map to an address for a single name. This is a clean, read-only use of the interior mutability and matches the trait signatures.If performance ever becomes a concern, you could revisit returning iterators or references instead of cloning the
Vec, but given this is CLI tooling, the current approach is perfectly acceptable.
284-303: Duplicate detection inContractsData::add_contractis correct and message is clear
ContractsData::add_contractnow:
- Constructs a
DeployedContractwith the providedpackage_name(or default ident).- Rejects duplicates based on
package_namewithContractError::ContractExists(contract.package_name).- Updates
last_updatedonly on successful insert.This enforces uniqueness on the key that the CLI uses (
package_name/key_name) and the error text (“Contract{0}already exists”) will now clearly reference the conflicting name. Implementation looks solid.
Benchmark report
|
kubaplas
force-pushed
the
feature/mutable-container-in-cli
branch
from
4e2423a to
3eb4045
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
odra-cli/src/container.rs (3)
213-222: Use the actual lookup name in theNotFounderror forcontract_ref_named
contract_ref_namedcomputes anamefrom either the providedpackage_nameorT::HostRef::ident(), but on failure it always returnsContractError::NotFound(T::HostRef::ident()). If a caller passes a custom name, the error message will not reflect what was actually looked up.You can make the error clearer by returning the effective
nameinstead:let name = package_name.unwrap_or(T::HostRef::ident()); self.data .borrow() .contracts() .iter() .find(|c| c.key_name() == name) .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())) + .ok_or(ContractError::NotFound(name))This way the error always reports the exact key the caller attempted to resolve.
291-310: Align duplicate-check withkey_name()to avoid subtle duplicates with legacy data
ContractsData::add_contractcurrently checks duplicates by comparingpackage_name:if self.contracts.iter().any(|c| c.package_name == contract.package_name) { return Err(ContractError::ContractExists(contract.package_name)); }However, lookups (
contract_ref_named,address_by_name) useDeployedContract::key_name(), which falls back tonamewhenpackage_nameis empty. If you ever load olderdeployed_contracts.tomlentries that have an emptypackage_namebut a non-emptyname, it’s possible to insert a new contract whosekey_name()collides with an existing one whilepackage_namedoes not, leading to ambiguous lookups instead of aContractExistserror.To make this robust and aligned with how contracts are actually addressed, consider deduplicating by the effective key name:
- pub fn add_contract<T: HasIdent>( - &mut self, - address: Address, - package_name: Option<String> - ) -> Result<(), ContractError> { - let contract = DeployedContract::new::<T>(address, package_name); - - // Check if a contract with this package_name already exists - if self - .contracts - .iter() - .any(|c| c.package_name == contract.package_name) - { - return Err(ContractError::ContractExists(contract.package_name)); - } + pub fn add_contract<T: HasIdent>( + &mut self, + address: Address, + package_name: Option<String> + ) -> Result<(), ContractError> { + let contract = DeployedContract::new::<T>(address, package_name); + let new_key_name = contract.key_name(); + + // Check if a contract with this effective key name already exists + if self + .contracts + .iter() + .any(|c| c.key_name() == new_key_name) + { + return Err(ContractError::ContractExists(new_key_name)); + } self.contracts.push(contract); self.last_updated = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true); Ok(()) }This keeps the duplicate rule consistent with how callers resolve contracts and also makes the
ContractExistspayload match the actual key being reserved.
135-137: Consider updatingapply_deploy_modesignature for better API ergonomicsThe interior mutability refactor for
DeployedContractsContaineris correct; borrow scopes are short and non-overlapping. Verified:apply_deploy_modereceives aStringfreshly allocated at the call site (viaread_arg::<String>) and doesn't require ownership. Changing the signature to accept&stris a valid improvement that avoids unnecessary ownership transfer while maintaining compatibility (callers can passStringvalues directly due to deref coercion). This would also make the API more flexible for future callers with borrowed strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odra-cli/src/cli.rs(1 hunks)odra-cli/src/container.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- odra-cli/src/cli.rs
🧰 Additional context used
🧬 Code graph analysis (1)
odra-cli/src/container.rs (2)
core/src/host.rs (5)
new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)odra-cli/src/utils.rs (1)
log(13-15)
🔇 Additional comments (1)
odra-cli/src/container.rs (1)
30-32: New error variants fit cleanly into the existing error modelAdding
SchemaFileNotFound(String)andContractExists(String)keeps errors explicit and consistent with the rest ofContractError. No issues here.
contracts in them.
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.