Feature/factory upgrade #620
Conversation
- Introduced a new module `entry_point` to define various types of entry points for smart contracts, including Regular, Constructor, Template, Factory, and Upgrader. - Implemented an extension trait `EntityEntryPointsExt` for adding entry points to `casper_types::EntryPoints`. - Refactored existing code in `odra-macros` to utilize the new entry point definitions, simplifying the process of adding entry points in smart contracts. - Updated the `lib.rs` file to include the new `entry_point` module. - Removed redundant code related to entry point creation and access management, streamlining the macro implementation.
…de and specific_args
…t.rs; clean up host_functions.rs for better readability and formatting.
…ions for factory caller override
WalkthroughAdds a typed EntryPoint API and re-exports it; introduces factory-focused entrypoint variants (new_contract, upgrade_child_contract, batch_upgrade_child_contract) and BatchUpgradeArgs; refactors macro codegen to support multi-context factory flows; updates wasm host to support factory-caller override and factory runtime flags; updates examples and schemas accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Factory as Factory Contract (new_contract)
participant Host as Wasm Host
participant Child as Child Contract
User->>Factory: new_contract(name, init_args)
Factory->>Host: install (installer + entry_points)
Host-->>Factory: returns (Child Address, URef)
User->>Factory: upgrade_child_contract(name, upgrade_args)
Factory->>Host: invoke factory_upgrade entry (sets IS_FACTORY_UPGRADE_ARG)
Host->>Host: override_factory_caller() (enable override)
Host->>Child: call child's upgrader with forwarded args
Child-->>Host: upgrade result
Host-->>Factory: success/failure
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (5)odra-macros/src/ast/factory/parts/ref_item.rs (4)
odra-macros/src/ast/factory/parts/test_parts.rs (6)
odra-macros/src/ast/schema/entry_points.rs (5)
odra-macros/src/ast/factory/impl_item.rs (5)
odra-macros/src/ast/factory/parts/wasm_parts.rs (8)
🔇 Additional comments (20)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
odra-macros/src/ast/factory/impl_item.rs (1)
319-384: Forward attached tokens intry_*factory calls.
try_new_contract,try_upgrade_child_contract, andtry_batch_upgrade_child_contractno longer forwardself.attached_valuebecause the new implementations callodra::CallDef::new(...)without chaining.with_amount(self.attached_value). The non-tryvariants still forward the tokens, so thetry_*helpers now behave differently (and will silently drop any value provided viawith_tokens). This will break paid factory flows and introduce hard-to-trace funding bugs. Please mirror the non-trylogic by adding.with_amount(self.attached_value)before passing theCallDeftocall_contract.- odra::CallDef::new( + odra::CallDef::new( odra::prelude::string::String::from("new_contract"), true, { let mut named_args = odra::casper_types::RuntimeArgs::new(); let _ = named_args.insert("contract_name", contract_name.clone()); let _ = named_args.insert("value", value.clone()); let _ = named_args.insert("odra_cfg_is_upgradable", true); let _ = named_args.insert("odra_cfg_is_upgrade", false); let _ = named_args.insert("odra_cfg_allow_key_override", true); let _ = named_args.insert("odra_cfg_package_hash_key_name", contract_name); named_args }, ) + .with_amount(self.attached_value)Repeat the same adjustment for
try_upgrade_child_contractandtry_batch_upgrade_child_contract.odra-casper/wasm-env/src/host_functions.rs (1)
72-1076: CALLER_OVERRIDE never resets—subsequent calls will misidentify (or panic on) caller
CALLER_OVERRIDEis flipped totruein several factory paths (install_new_contract,override_factory_caller) but there is no code that clears it afterwards. Once any factory deploy/upgrade runs, every latercaller()invocation in the process will keep readingtake_nth_caller_from_stack(2). Two bad things follow:
- Regular entrypoints (depth 2 stack) now panic because
nth_back(2)unwrapsNone.- Even when the stack is deep enough, non-factory code now sees the wrong caller (the grandparent frame) forever.
We need symmetrical teardown of this override (e.g. RAII guard / reference counter) so the flag is cleared when the factory-specific section finishes, otherwise the very next contract call can fail or get bogus caller info. Please add the corresponding reset logic and update call sites accordingly.
🧹 Nitpick comments (2)
odra-casper/wasm-env/Cargo.toml (1)
16-16: Minor formatting inconsistency.There's an extra space after the comma in the dependency specification. While this doesn't affect functionality, it's inconsistent with formatting conventions.
Apply this diff to fix the formatting:
-casper-contract = { workspace = true, default-features = false } +casper-contract = { workspace = true, default-features = false }odra-macros/src/ast/ref_utils.rs (1)
108-126: Replace hardcoded strings with constants.The runtime argument names are hardcoded as strings, but corresponding constants exist in
core/src/consts.rs(e.g.,IS_FACTORY_UPGRADE_ARG,ALLOW_KEY_OVERRIDE_ARG,CREATE_UPGRADE_GROUP, etc.). Using the constants would prevent typos and improve maintainability.Consider refactoring to use the defined constants. For example:
use odra_core::consts::*; // Then in the match expression: FnType::FactoryUpgrader => quote::quote! { let _ = #args.insert(IS_FACTORY_UPGRADE_ARG, true); let _ = #args.insert(ALLOW_KEY_OVERRIDE_ARG, true); let _ = #args.insert(CREATE_UPGRADE_GROUP, false); }, // ... similar for other armsNote: You may need to adjust how constants are imported/used in the proc-macro context depending on your build setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
core/src/consts.rs(1 hunks)core/src/entry_point.rs(1 hunks)core/src/lib.rs(1 hunks)examples/.cargo/config.toml(1 hunks)examples/Odra.toml(1 hunks)examples/bin/factory_example.rs(1 hunks)examples/src/factory/counter.rs(3 hunks)examples/src/factory/mod.rs(1 hunks)examples/src/factory/token.rs(1 hunks)odra-casper/test-vm/src/vm/casper_vm.rs(1 hunks)odra-casper/wasm-env/Cargo.toml(1 hunks)odra-casper/wasm-env/src/consts.rs(1 hunks)odra-casper/wasm-env/src/host_functions.rs(11 hunks)odra-macros/src/ast/factory/impl_item.rs(13 hunks)odra-macros/src/ast/factory/mod.rs(1 hunks)odra-macros/src/ast/factory/parts/deployer_item.rs(1 hunks)odra-macros/src/ast/factory/parts/has_entrypoints_item.rs(2 hunks)odra-macros/src/ast/factory/parts/host_ref_item.rs(1 hunks)odra-macros/src/ast/factory/parts/ref_item.rs(3 hunks)odra-macros/src/ast/factory/parts/test_parts.rs(4 hunks)odra-macros/src/ast/factory/parts/wasm_parts.rs(23 hunks)odra-macros/src/ast/ref_utils.rs(2 hunks)odra-macros/src/ast/schema/entry_points.rs(1 hunks)odra-macros/src/ast/wasm_parts.rs(11 hunks)odra-macros/src/ast/wasm_parts_utils.rs(2 hunks)odra-macros/src/ir/mod.rs(4 hunks)odra-macros/src/utils/expr.rs(1 hunks)odra-macros/src/utils/ident.rs(1 hunks)odra-macros/src/utils/ty.rs(1 hunks)odra/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
core/src/lib.rs (1)
core/src/entry_point.rs (1)
entry_point(73-86)
odra/src/lib.rs (3)
core/src/entry_point.rs (1)
entry_point(73-86)odra-macros/src/utils/ty.rs (1)
entry_point(80-82)odra-schema/src/lib.rs (2)
entry_point(79-94)entry_point(338-338)
odra-macros/src/ast/schema/entry_points.rs (3)
odra-macros/src/utils/expr.rs (1)
vec(145-147)odra-macros/src/utils/ty.rs (3)
vec(196-198)schema(99-101)string(167-169)odra-schema/src/lib.rs (6)
schema(169-222)schema(456-456)argument(70-76)argument(323-323)argument(330-330)argument(337-337)
odra-macros/src/utils/ty.rs (2)
odra-macros/src/utils/ident.rs (1)
bytes(90-92)core/src/call_result.rs (1)
bytes(45-52)
odra-macros/src/ast/wasm_parts_utils.rs (3)
odra-macros/src/utils/expr.rs (1)
vec(145-147)odra-macros/src/utils/ty.rs (2)
vec(196-198)entry_point(80-82)core/src/entry_point.rs (1)
entry_point(73-86)
examples/src/factory/counter.rs (3)
odra-macros/src/ast/factory/parts/wasm_parts.rs (4)
module(74-79)module(161-165)module(166-170)module(406-411)core/src/entry_point.rs (2)
factory(114-123)from(60-70)core/src/host.rs (18)
InstallConfig(187-187)InstallConfig(227-227)env(33-33)deploy(80-80)deploy(206-218)address(45-47)address(1038-1040)from(134-136)deploy_with_cfg(90-90)deploy_with_cfg(231-244)upgradable(186-188)try_upgrade(102-106)try_upgrade(267-278)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)
odra-macros/src/ast/factory/parts/host_ref_item.rs (3)
odra-macros/src/ir/mod.rs (8)
syn(204-204)syn(205-205)syn(209-209)syn(470-470)syn(819-819)factory_fn(361-371)factory_upgrade_fn(373-380)factory_batch_upgrade_fn(382-388)odra-macros/src/ast/factory/parts/ref_item.rs (2)
try_from(30-36)to_tokens(40-48)odra-macros/src/ast/ref_utils.rs (5)
try_from(220-224)host_function_item(31-46)to_tokens(228-236)to_tokens(254-262)factory_try_function_item(21-29)
odra-macros/src/utils/expr.rs (2)
core/src/entry_point.rs (1)
entry_point(73-86)odra-macros/src/utils/ty.rs (1)
entry_point(80-82)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (4)
odra-macros/src/utils/expr.rs (2)
vec(145-147)args(169-172)odra-macros/src/utils/ty.rs (4)
vec(196-198)string(167-169)from(216-218)cl_type(111-113)odra-macros/src/ir/mod.rs (6)
factory_fn(361-371)factory_upgrade_fn(373-380)factory_batch_upgrade_fn(382-388)name(602-604)name(771-779)ty(785-790)core/src/entry_point.rs (1)
from(60-70)
odra-macros/src/ast/factory/parts/deployer_item.rs (2)
core/src/entry_point.rs (1)
entry_point(73-86)odra-macros/src/utils/ty.rs (1)
entry_point(80-82)
odra-macros/src/ast/factory/parts/test_parts.rs (3)
odra-casper/test-vm/src/vm/casper_vm.rs (5)
new_contract(440-464)value(815-820)value(827-832)call_contract(308-378)new(75-77)odra-macros/src/utils/ty.rs (6)
string(167-169)address(4-6)from(216-218)vec(196-198)call_def(44-46)entry_point(80-82)core/src/entry_point.rs (2)
from(60-70)entry_point(73-86)
odra-macros/src/ir/mod.rs (3)
odra-macros/src/utils/ty.rs (4)
address(4-6)uref(72-74)string(167-169)bytes(276-278)core/src/host.rs (4)
address(45-47)address(1038-1040)new_contract(385-390)new_contract(543-560)odra-vm/src/vm/odra_vm.rs (2)
new_contract(42-63)new(37-39)
odra-macros/src/ast/factory/impl_item.rs (3)
core/src/entry_point.rs (4)
from(60-70)entry_point(73-86)add(50-50)add(54-56)core/src/host.rs (11)
from(134-136)new_contract(385-390)new_contract(543-560)env(33-33)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)upgrade_contract(394-400)upgrade_contract(564-588)odra-casper/wasm-env/src/host_functions.rs (7)
override_factory_caller(1072-1076)get_named_arg(325-350)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)upgrade_contract(196-312)
core/src/entry_point.rs (3)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (2)
vec(78-117)vec(99-102)odra-macros/src/utils/expr.rs (2)
vec(145-147)args(169-172)odra-macros/src/utils/ty.rs (4)
vec(196-198)from(216-218)entry_point(80-82)cl_type(111-113)
odra-macros/src/ast/ref_utils.rs (1)
odra-macros/src/utils/expr.rs (2)
args(169-172)new_runtime_args(4-7)
odra-macros/src/ast/factory/parts/wasm_parts.rs (6)
odra-macros/src/ir/mod.rs (16)
syn(204-204)syn(205-205)syn(209-209)syn(470-470)syn(819-819)host_functions(350-352)factory_upgrade_fn(373-380)ty(785-790)upgrader_args(413-420)name(602-604)name(771-779)named_args(636-641)new(719-721)new(737-739)new(763-765)contract_name(137-143)odra-macros/src/ast/wasm_parts_utils.rs (2)
func(8-14)use_entity_entry_points_ext(35-37)odra-macros/src/utils/ident.rs (8)
ident(86-88)result(11-13)entry_points(47-49)child_contract_entry_points(51-53)schemas(59-61)event_schemas(74-76)named_args(3-5)env(19-21)odra-macros/src/utils/expr.rs (11)
new_wasm_contract_env(73-75)args(169-172)template_ep(235-241)constructor_ep(201-205)upgrader_ep(207-211)regular_ep(213-219)factory_ep(221-225)factory_upgrade_ep(227-229)factory_batch_upgrade_ep(231-233)schemas(68-71)event_schemas(86-89)odra-macros/src/utils/stmt.rs (1)
runtime_return(4-12)odra-casper/wasm-env/src/host_functions.rs (5)
override_factory_caller(1072-1076)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)
odra-macros/src/ast/factory/parts/ref_item.rs (2)
odra-macros/src/ir/mod.rs (12)
syn(204-204)syn(205-205)syn(209-209)syn(470-470)syn(819-819)factory_fn(361-371)factory_upgrade_fn(373-380)factory_batch_upgrade_fn(382-388)contract_name(137-143)new(719-721)new(737-739)new(763-765)odra-macros/src/ast/ref_utils.rs (2)
fun(99-107)contract_function_item(48-58)
odra-casper/wasm-env/src/host_functions.rs (4)
core/src/call_def.rs (1)
args(77-79)odra-vm/src/vm/odra_vm.rs (2)
get_named_arg(185-194)caller(162-164)odra-vm/src/vm/odra_vm_state.rs (1)
caller(59-61)odra-casper/wasm-env/src/wasm_contract_env.rs (1)
caller(51-53)
odra-macros/src/ast/wasm_parts.rs (5)
odra-macros/src/ast/wasm_parts_utils.rs (4)
use_entity_entry_points_ext(35-37)func(8-14)param_parameters(7-16)param_ret_ty(18-23)odra-macros/src/utils/attr.rs (1)
no_mangle(15-17)odra-macros/src/utils/expr.rs (6)
args(169-172)constructor_ep(201-205)upgrader_ep(207-211)regular_ep(213-219)default(162-164)vec(145-147)odra-macros/src/utils/ident.rs (4)
ident(86-88)entry_points(47-49)add_entry_point(55-57)cl_type(106-108)core/src/entry_point.rs (3)
entry_point(73-86)add(50-50)add(54-56)
examples/bin/factory_example.rs (2)
examples/src/factory/counter.rs (3)
InstallConfig(106-106)InstallConfig(166-166)InstallConfig(228-228)core/src/entry_point.rs (2)
factory(114-123)from(60-70)
⏰ 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: Calculate test coverage
- GitHub Check: Test
- GitHub Check: Evaluate benchmark
- GitHub Check: Benchmark
🔇 Additional comments (25)
examples/.cargo/config.toml (1)
4-5: No issues found — cargo alias is properly configured.The
livenetfeature is defined inexamples/Cargo.tomlaslivenet = ["odra-casper-livenet-env"]and is actively used by binaries likeerc20_on_livenetandour_token_livenet. The alias will work correctly.odra-casper/test-vm/src/vm/casper_vm.rs (1)
938-938: Implicit capture keeps behavior intact.Switching to Rust’s implicit capture formatting preserves the original error message semantics while matching newer style.
odra-macros/src/ast/wasm_parts.rs (2)
90-97: Trait import keeps add() resolution workingBringing
EntityEntryPointsExtinto scope is the key piece that lets the generated.add(...)calls resolve against the new enum variants instead of the inherent Casper API. Nicely handled.
192-214: Generic NoMangle wrapper looks solidThe new context-parameterized
NoMangleFnItemcleanly future-proofs the codegen path while keeping the emitted bodies unchanged today. Good call.core/src/consts.rs (1)
57-58: LGTM!The new constant follows the established naming convention and is properly documented. This constant will help maintain consistency when referencing the factory upgrade argument throughout the codebase.
examples/Odra.toml (1)
86-95: LGTM!The new contract entries align with the factory upgrade feature additions. The configuration correctly includes the factory contracts and their child contracts.
odra-macros/src/ast/factory/parts/deployer_item.rs (1)
100-100: LGTM!The entry point name change from "factory" to "new_contract" is consistent with the broader refactoring in this PR.
odra/src/lib.rs (1)
41-41: LGTM!Adding
entry_pointto the public exports enables consumers to utilize the new entry point infrastructure introduced in this PR.odra-macros/src/ast/schema/entry_points.rs (1)
304-310: LGTM!The entry point rename to "new_contract" and the argument type change to
odra::prelude::string::Stringare both consistent with the factory upgrade feature. The use of the prelude String type ensures no_std compatibility.odra-macros/src/ast/factory/parts/host_ref_item.rs (2)
22-49: LGTM!The refactoring from a single
factory_fnto a vector offactory_fnscorrectly supports the new factory upgrade entry points (new_contract,upgrade_child_contract,batch_upgrade_child_contract). The implementation properly generates and emits all three functions.
51-79: LGTM!The try implementation mirror correctly handles all three factory entry points, maintaining consistency with the non-try implementation above.
odra-macros/src/ast/factory/parts/ref_item.rs (2)
22-36: LGTM!The refactoring from a single
factory_fnto a collection offactory_fnsis well-structured and aligns with the PR objective of adding factory upgrade capabilities. The three functions (factory_fn,factory_upgrade_fn,factory_batch_upgrade_fn) are properly collected and mapped.
39-49: LGTM!The
ToTokensimplementation correctly uses repetition syntax to emit all factory functions.odra-macros/src/ir/mod.rs (4)
16-29: LGTM!The introduction of named constants for factory-related entry points and the
FnTypeenum provides excellent type safety and maintainability. The public visibility ofFnTypeis appropriate for downstream usage.
361-388: LGTM!The three factory-related function generators are well-designed:
factory_fnreuses constructor arguments for creating new contractsfactory_upgrade_fnreuses upgrader arguments for single upgradesfactory_batch_upgrade_fnuses serializedBytesparameters for flexible batch operations
530-540: LGTM!The
fn_type()method provides clean function classification based on name matching against the defined constants.
543-550: LGTM!The
PROTECTED_FUNCTIONSarray correctly includes the three new factory-related entry point names, preventing naming conflicts.odra-macros/src/ast/factory/parts/test_parts.rs (2)
119-134: LGTM!The public convenience methods follow the standard pattern of unwrapping the
try_*variants, providing a clean API for test scenarios where errors are unexpected.
139-215: LGTM!The
try_*methods correctly construct runtime arguments with appropriate configuration flags:
try_new_contractmarks contracts as upgradable and not an upgrade operationtry_upgrade_child_contractandtry_batch_upgrade_child_contractboth setodra_cfg_is_factory_upgradeto distinguish factory-initiated upgradesodra-macros/src/ast/factory/parts/wasm_parts.rs (6)
15-46: LGTM!The
TryFromimplementation forNoMangleFnItem<FactoryContext>correctly injects the caller override logic, which is essential for factory operations to execute with the proper caller context.
113-154: LGTM!The dual entry point generation is well-designed:
entry_points()for the factory contract itself (with template entry points and factory operations)child_contract_entry_points()for contracts created by the factory (with standard constructor/regular entry points)This separation enables proper contract installation for both the factory and its child contracts.
178-273: LGTM!The context-based entry point generation provides excellent type safety and clear separation of concerns. Each context type generates the appropriate entry point variant for its use case.
319-392: LGTM!The
new_contractfunction generation is comprehensive and correct:
- Properly overrides the factory caller context
- Uses
child_contract_entry_points()for the installed contract- Emits a deployment event
- Returns both the contract address and access URef
442-476: LGTM!The
upgrade_child_contractfunction correctly:
- Retrieves the existing contract by name
- Extracts its package hash
- Constructs upgrade arguments including the target package hash
- Performs the upgrade using
upgrade_contract- Emits a deployment event
499-518: Verify error handling for missing contracts in batch upgrade.The batch upgrade implementation silently skips contracts that don't exist (line 501:
if let Some(key) = contract_key). While this could be intentional to allow partial upgrades, it may also hide configuration errors where a contract name is misspelled or the contract doesn't exist.Consider whether the batch upgrade should:
- Fail fast on the first missing contract (stricter)
- Log/emit events for skipped contracts (informative)
- Continue silently as currently implemented (permissive)
Based on learnings: This verification aligns with the error handling patterns established in the codebase.
![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 introduces a factory upgrade system but contains critical security and correctness issues, including global caller override risks, missing upgrade arguments, and hardcoded contract dependencies that could break existing implementations.
📄 Documentation Diagram
This diagram documents the new factory upgrade workflow for child contracts.
sequenceDiagram
participant User
participant Factory
participant ChildContract
User->>Factory: Call upgrade_child_contract
note over Factory: PR #35;620 added upgrade logic
Factory->>ChildContract: Call upgrade with new args
ChildContract-->>Factory: Return
Factory-->>User: Return
| Priority | File | Category | Impact Summary |
|---|---|---|---|
| P1 | odra-casper/.../host_functions.rs | Architecture | Security context change via global caller override |
| P1 | odra-macros/.../ref_utils.rs | Correctness | Missing package hash key in upgrade args |
| P1 | odra-casper/.../host_functions.rs | Security | Race condition from global mutable state |
| P1 | odra-macros/.../wasm_parts.rs | Architecture | Hardcoded child contract event schemas |
| P2 | odra-casper/.../host_functions.rs | Correctness | Incorrect upgrade detection logic |
| P2 | odra-macros/.../wasm_parts.rs | Robustness | Silent failures in batch upgrades |
| P2 | examples/.../counter.rs | Documentation | Missing access control in upgrade function |
| P2 | odra-macros/.../mod.rs | Maintainability | Potential naming conflicts in protected functions |
🔍 Notable Themes
- Security Isolation: Multiple findings highlight risks in cross-contract caller resolution and global state management, violating contract isolation principles.
- Upgrade Configuration: Critical missing arguments and hardcoded dependencies undermine the factory pattern's flexibility and correctness.
- Error Handling: Batch operations lack robust error reporting, potentially leading to silent failures in production environments.
📈 Risk Diagram
This diagram illustrates the security risk introduced by the global caller override mechanism.
sequenceDiagram
participant User
participant Factory
participant ChildContract
User->>Factory: Call method
note over Factory: R1(P1): Global CALLER_OVERRIDE flag set
Factory->>ChildContract: Call method
note over ChildContract: R1(P1): Sees User as caller due to override
ChildContract-->>Factory: Return
Factory-->>User: Return
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: odra-macros/src/ast/factory/parts/wasm_parts.rs
The batch upgrade silently skips contracts that aren't found in the named keys. This could lead to silent failures where expected upgrades don't occur. The system should either revert the entire batch or provide clear error reporting for missing contracts.
Suggestion:
for name in names_to_upgrade {
let contract_key = runtime::get_key(&name)
.unwrap_or_revert_with(ExecutionError::ContractNotFound(name.clone()));
// Proceed with upgrade
}Related Code:
for name in names_to_upgrade {
let contract_key = runtime::get_key(&name);
if let Some(key) = contract_key {
// Proceed with upgrade
}
// No error handling for missing contracts
}📁 File: odra-macros/src/ir/mod.rs
The protected function names now include factory-specific methods, but the naming convention (new_contract) may conflict with user-defined functions in non-factory contracts. This could cause confusion and naming collisions in complex contract systems.
Related Code:
const PROTECTED_FUNCTIONS: [&str; 6] = [
"new", "env", "address", "new_contract",
"upgrade_child_contract", "batch_upgrade_child_contract"
];💡 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
examples/src/factory/counter.rs (4)
24-47: Code duplication is acceptable for example purposes.
BetterCounterduplicates most ofCounter's implementation, differing only in theupgrademethod. While this violates DRY principles, it's reasonable for example code demonstrating upgrade flows.If this were production code, consider extracting shared logic into a common trait or base implementation.
122-149: Consider extracting serialization helpers for readability.The batch upgrade serialization logic is verbose and deeply nested. Extracting helper functions would improve readability and reusability across tests.
For example:
fn serialize_runtime_args(args: RuntimeArgs) -> Bytes { Bytes::from( args.to_bytes() .expect("Failed to serialize runtime args") ) } fn serialize_upgrade_names(names: Vec<String>) -> Bytes { Bytes::from( names.to_bytes() .expect("Failed to serialize names") ) } fn serialize_specific_args(args: BTreeMap<String, RuntimeArgs>) -> Bytes { Bytes::from( args.to_bytes() .expect("Failed to serialize specific args") ) }Then line 122 becomes:
factory.batch_upgrade_child_contract( serialize_runtime_args(runtime_args! { "new_value" => 42u32 }), serialize_upgrade_names(vec!["FromTwo".to_string(), "FromThree".to_string(), "FromHundred".to_string()]), serialize_specific_args(BTreeMap::from([("FromHundred".to_string(), runtime_args! { "new_value" => 1000u32 })])) );
178-178: Replace magic number with named constant or add explanation.The account index
11lacks context. Consider using a named constant or adding a comment explaining why this specific account is used.+ // Use a different account (not the deployer) to simulate unauthorized access let unauthorized_account = env.get_account(11);
223-267: LGTM: Good error handling test.This test properly validates that incorrect upgrade arguments are caught with
MissingArgerror. The comment on line 247 clearly documents the intentional mistake.The serialization code here would also benefit from the helper functions suggested for
test_factory_upgrade_works.examples/src/factory/token.rs (1)
66-85: Consider the ignored test.The factory test is permanently disabled with a note that it doesn't work on the Odra VM. Consider either:
- Removing the test if factory functionality won't be supported on Odra VM
- Adding a comment explaining when/if this limitation will be resolved
- Keeping it as documentation of intended behavior for other environments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/src/factory/counter.rs(3 hunks)examples/src/factory/token.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/src/factory/counter.rs (3)
odra-macros/src/ast/factory/parts/wasm_parts.rs (4)
module(74-79)module(161-165)module(166-170)module(406-411)core/src/entry_point.rs (2)
factory(114-123)from(60-70)core/src/host.rs (16)
InstallConfig(187-187)InstallConfig(227-227)address(45-47)address(1038-1040)from(134-136)env(33-33)deploy_with_cfg(90-90)deploy_with_cfg(231-244)upgradable(186-188)try_upgrade(102-106)try_upgrade(267-278)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)
examples/src/factory/token.rs (1)
examples/src/factory/counter.rs (3)
init(11-13)init(32-34)test_standalone_module(68-74)
🔇 Additional comments (4)
examples/src/factory/counter.rs (2)
52-59: LGTM: Imports support the new upgrade tests.The added Casper types and
BetterCounterFactoryare necessary for the comprehensive upgrade test scenarios.Also applies to: 61-61
83-83: LGTM: Updated factory method call.The rename to
new_contractimproves API clarity and properly captures both the address and access URef.examples/src/factory/token.rs (2)
19-31: LGTM!The delegation pattern correctly forwards token operations to the
Cep18submodule and ownership queries to theOwnablesubmodule. This is a clean way to compose functionality.
13-14: The argument order is correct—no issue here.The swap is intentional. The underlying
Cep18::initexpects parameters in order:symbol: String, name: String, decimals: u8, initial_supply: U256, while the public API inFToken::initpresents them asname, symbol, decimals, initial_supplyfor a more intuitive interface. The code correctly adapts between these two orders when delegating to the implementation.
Benchmark report
|
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
odra-macros/src/ast/factory/impl_item.rs (1)
149-213: ContractRef upgrade methods miss factory-upgrade flags, breaking cross-contract factory upgrades
Erc20FactoryContractRef::upgrade_child_contractandbatch_upgrade_child_contractonly pass user-facing arguments (contract_name,default_args,names_to_upgrade,specific_args) plus optionalamount. They do not insert:
odra_cfg_is_factory_upgradeodra_cfg_allow_key_overrideodra_cfg_create_upgrade_groupIn contrast, the host-side
Erc20FactoryHostRef::try_upgrade_child_contractandtry_batch_upgrade_child_contractdo insert these flags into the runtime args, whichodra_casper_wasm_env::host_functions::upgrade_contractrelies on to select the “factory upgrade” branch and to decide whether key override and upgrade group creation are allowed.Implication:
- When
upgrade_child_contract/batch_upgrade_child_contractare invoked viaErc20FactoryContractReffrom another contract, the runtime will not seeodra_cfg_is_factory_upgrade = true.upgrade_contractwill take the non-factory path and attempt to read package-hash/key information from runtime args that are not present, causing the upgrade to revert instead of behaving like a factory upgrade.- The same factory upgrade invoked via
Erc20FactoryHostRefwill work, leading to inconsistent behavior between host-driven and contract-driven upgrades.You probably want ContractRef-based callers to behave like the host ref and request a factory upgrade as well.
Consider aligning the ContractRef implementations with the host ref by inserting these flags into
named_args, e.g.:pub fn upgrade_child_contract(&mut self, contract_name: odra::prelude::string::String) { self.env .call_contract( self.address, odra::CallDef::new( odra::prelude::string::String::from("upgrade_child_contract"), true, { let mut named_args = odra::casper_types::RuntimeArgs::new(); if self.attached_value > odra::casper_types::U512::zero() { let _ = named_args.insert("amount", self.attached_value); } odra::args::EntrypointArgument::insert_runtime_arg( contract_name.clone(), "contract_name", &mut named_args, ); + let _ = named_args.insert("odra_cfg_is_factory_upgrade", true); + let _ = named_args.insert("odra_cfg_allow_key_override", true); + let _ = named_args.insert("odra_cfg_create_upgrade_group", false); named_args }, ) .with_amount(self.attached_value), ) } pub fn batch_upgrade_child_contract( &mut self, default_args: odra::casper_types::bytesrepr::Bytes, names_to_upgrade: odra::casper_types::bytesrepr::Bytes, specific_args: odra::casper_types::bytesrepr::Bytes ) { self.env .call_contract( self.address, odra::CallDef::new( odra::prelude::string::String::from("batch_upgrade_child_contract"), true, { let mut named_args = odra::casper_types::RuntimeArgs::new(); if self.attached_value > odra::casper_types::U512::zero() { let _ = named_args.insert("amount", self.attached_value); } odra::args::EntrypointArgument::insert_runtime_arg( default_args.clone(), "default_args", &mut named_args, ); odra::args::EntrypointArgument::insert_runtime_arg( names_to_upgrade.clone(), "names_to_upgrade", &mut named_args, ); odra::args::EntrypointArgument::insert_runtime_arg( specific_args.clone(), "specific_args", &mut named_args, ); + let _ = named_args.insert("odra_cfg_is_factory_upgrade", true); + let _ = named_args.insert("odra_cfg_allow_key_override", true); + let _ = named_args.insert("odra_cfg_create_upgrade_group", false); named_args }, ) .with_amount(self.attached_value), ) }You’d then need to update the corresponding golden expectations in
ref_item.rsand related tests to match.Also applies to: 273-365
🧹 Nitpick comments (1)
odra-macros/src/ast/factory/impl_item.rs (1)
466-503: Wasm factory entrypoints use the new typed API correctly; consider caller-override consistency for upgrade flowsPositives:
entry_points()now usesodra::entry_point::EntryPoint::{Template, Factory, FactoryUpgrade, FactoryBatchUpgrade}plusEntityEntryPointsExt::add, aligning withcore::entry_point::EntryPointand keeping the mapping centralized.child_contract_entry_points()cleanly defines the constructor and regular Erc20 entry points for child contracts.new_contract,init,total_supply,pay_to_mint, andapprovenow calloverride_factory_caller()before delegating to exec parts / install logic, which should fix caller semantics when invoked through the factory.- The new
upgrade_child_contractandbatch_upgrade_child_contractwasm functions correctly:
- Resolve the package hash from
contract_name.- Build upgrade-specific
RuntimeArgs(odra_cfg_package_hash_to_upgrade,odra_cfg_package_hash_key_nameplus decoded default/specific args).- Call
host_functions::upgrade_contractwithchild_contract_entry_points()and re-emitErc20FactoryContractDeployedevents.One thing to double-check: unlike the other factory-related entrypoints,
upgrade_child_contractandbatch_upgrade_child_contractdo not calloverride_factory_caller()before constructingExecutionEnv. Ifoverride_factory_caller()is intended to ensure a consistentcaller()view for all factory-mediated operations (including upgrades), you may want to invoke it at the start of these two functions as well, for symmetry withnew_contractand the proxied Erc20 entrypoints.Also applies to: 505-532, 552-588, 590-688, 690-717
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
odra-macros/src/ast/factory/impl_item.rs(13 hunks)odra-macros/src/ast/factory/parts/ref_item.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
odra-macros/src/ast/factory/parts/ref_item.rs (3)
odra-macros/src/ir/mod.rs (13)
syn(204-204)syn(205-205)syn(209-209)syn(470-470)syn(819-819)factory_fn(361-371)factory_upgrade_fn(373-380)factory_batch_upgrade_fn(382-388)contract_name(137-143)new(719-721)new(737-739)new(763-765)named_args(636-641)odra-macros/src/ast/factory/parts/wasm_parts.rs (10)
module(74-79)module(161-165)module(166-170)module(406-411)to_tokens(85-110)to_tokens(122-153)to_tokens(199-206)to_tokens(280-306)to_tokens(341-391)to_tokens(422-521)odra-macros/src/ast/ref_utils.rs (4)
fun(99-107)contract_function_item(48-58)to_tokens(228-236)to_tokens(254-262)
odra-macros/src/ast/factory/impl_item.rs (4)
odra-macros/src/utils/ty.rs (9)
string(167-169)from(216-218)vec(196-198)address(4-6)call_def(44-46)entry_point(80-82)entry_points(76-78)schemas(96-98)schema(99-101)core/src/host.rs (16)
from(134-136)new_contract(385-390)new_contract(543-560)env(33-33)call_contract(377-382)call_contract(606-618)address(45-47)address(1038-1040)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)upgrade_contract(394-400)upgrade_contract(564-588)contract_address(31-31)core/src/entry_point.rs (4)
from(60-70)entry_point(73-86)add(50-50)add(54-56)odra-casper/wasm-env/src/host_functions.rs (8)
call_contract(574-595)override_factory_caller(1072-1076)get_named_arg(325-350)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)upgrade_contract(196-312)
⏰ 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 (5)
odra-macros/src/ast/factory/parts/ref_item.rs (2)
24-35: Vector-based aggregation of factory functions looks correct
ContractRefImplItemnow collectsfactory_fn,factory_upgrade_fn, andfactory_batch_upgrade_fnintofactory_fns: Vec<ItemFn>and emits them with#(#fns)*. This is a clean way to keep the ref implementation in sync with the IR while making it easy to extend with more factory-related functions later. No issues spotted.Also applies to: 39-46
90-177: ContractRef golden output matches new factory APIThe expected
Erc20FactoryContractRefmethods (new_contract,upgrade_child_contract,batch_upgrade_child_contract) correctly reflect:
- The renamed entry point (
"new_contract"instead of"factory").- The updated
contract_nametype (odra::prelude::string::String).- The additional upgrade-related entry points and their arguments.
This lines up with the IR (
factory_fn,factory_upgrade_fn,factory_batch_upgrade_fn) and the host/test wiring inimpl_item.rs. Once you adjust the actual ContractRef implementation for any future changes (e.g., extra config flags), remember to update this expectation in lockstep.odra-macros/src/ast/factory/impl_item.rs (3)
72-118: Factory entrypoints and schema rewiring tonew_contractlook consistent
HasEntrypointsnow exposes:
new_contract(contract_name: odra::prelude::string::String, value: u32) -> (Address, URef).upgrade_child_contract(contract_name: odra::prelude::string::String).batch_upgrade_child_contract(default_args, names_to_upgrade, specific_args: Bytes).
SchemaEntrypointssimilarly renames"factory"to"new_contract"and updatescontract_nametoodra::prelude::string::String. This matches the IR (factory_fn,factory_upgrade_fn,factory_batch_upgrade_fn) and the wasm-side entry point mapping. No problems here.Also applies to: 739-761
296-317: HostReftry_new_contractwiring and flags look coherent
Erc20FactoryHostRef::try_new_contractnow:
- Calls
"new_contract"instead of"factory".- Uses
odra::prelude::string::Stringforcontract_name.- Injects
odra_cfg_is_upgradable,odra_cfg_is_upgrade = false,odra_cfg_allow_key_override = true, andodra_cfg_package_hash_key_name = contract_name.This matches the factory-install path expectations and ensures that new contracts created via the factory are upgradable with a predictable key name. No change requested; just noting that these defaults (especially always allowing key override) are now baked into the host helper.
373-405: EntryPointsCallerProvider update tonew_contractis consistentThe
EntryPointsCallerProviderforErc20FactoryHostRefnow exposes a singleEntryPoint::new("new_contract", [contract_name, value])and explicitly rejects such calls with a clear error message, mirroring the previous behavior for"factory". This keeps host-side entry-point discovery in sync with the rename while preserving the “factory not supported for this configuration” safeguard.
- Introduced `upgrade_child_contract` and `batch_upgrade_child_contract` entry points in the BetterCounterFactory and FTokenFactory schemas. - Updated the existing `new_contract` entry point to reflect the new naming convention. - Created new JSON schema files for BetterCounterFactory and FTokenFactory, detailing their structure and events. - Enhanced the FactoryModuleImplItem to include the new entry points in the test module. - Updated the FactorySchemaEntrypointsItem to include the new upgrade functions in the entry point collection.
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
odra-macros/src/ir/mod.rs (1)
404-420: Makeupgrader_args()private to matchconstructor_args().Both methods follow identical patterns and are only used internally (at lines 362 and 374). The visibility inconsistency was flagged in a previous review. Change
pub fn upgrader_args()tofn upgrader_args()at line 413 for consistency.Apply this diff:
- pub fn upgrader_args(&self) -> syn::punctuated::Punctuated<syn::FnArg, syn::token::Comma> { + fn upgrader_args(&self) -> syn::punctuated::Punctuated<syn::FnArg, syn::token::Comma> {Based on previous review.
🧹 Nitpick comments (4)
odra-macros/src/ast/factory/impl_item.rs (3)
389-399: Consider updating the error message for clarity.The entry point has been renamed from
factorytonew_contract, but the error message still references "Factory". Consider updating it to "Contract creation is not supported for this configuration" or similar for better clarity.Apply this diff to improve the error message:
return Err( OdraError::VmError( odra::VmError::Other( odra::prelude::String::from( - "Factory is not supported for this configuration.", + "Contract creation is not supported for this configuration.", ), ), ), );
590-688: Consider refactoring to reduce code duplication.There's significant duplication between
upgrade_child_contract(lines 590-627) andbatch_upgrade_child_contract(lines 629-688):
- Schema creation (lines 593-595 vs 633-635)
- Execution environment setup (lines 596-600 vs 636-640)
- Package hash insertion logic (lines 614-615 vs 673-674)
Additionally,
schemas.clone()on line 677 occurs inside the loop but doesn't need to since schemas are immutable.Consider extracting common logic into helper functions:
#[inline] fn setup_child_contract_env() -> (odra::casper_event_standard::Schemas, odra::ExecutionEnv) { let schemas = odra::casper_event_standard::Schemas( <Erc20 as odra::contract_def::HasEvents>::event_schemas() ); let exec_env = { let env = odra::odra_casper_wasm_env::WasmContractEnv::new_env(); let env_rc = Rc::new(env); odra::ExecutionEnv::new(env_rc) }; (schemas, exec_env) }For line 677, move
schemas.clone()outside the loop or pass&schemasif the API permits.
672-672: Minor: Consider optimizing default_args usage.
default_args.clone()is called for each contract without specific arguments. If most contracts use default arguments, consider cloning once before the loop and reusing that clone.+ let default_args_for_upgrades = default_args.clone(); for name in names_to_upgrade { let contract_key = odra::odra_casper_wasm_env::casper_contract::contract_api::runtime::get_key(&name); if let Some(key) = contract_key { let package_hash = UnwrapOrRevert::unwrap_or_revert(key.into_package_hash()); - let mut named_args = specific_args.get(&name).cloned().unwrap_or_else(|| default_args.clone()); + let mut named_args = specific_args.get(&name).cloned().unwrap_or_else(|| default_args_for_upgrades.clone());However, this is a minor optimization and may not provide significant benefits depending on the typical number of contracts and argument sizes.
examples/src/factory/token.rs (1)
20-32: Delegation set is minimal but consistent; consider exposing decimals if neededDelegating core CEP‑18 methods (
transfer,balance_of,total_supply,name,symbol) andget_owneris consistent and keeps the surface small. If external callers needdecimals()or other CEP‑18 helpers, you might also delegate those here for a more complete token API, but it’s fine as‑is if intentionally minimal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/resources/casper_contract_schemas/better_counter_factory_schema.json(1 hunks)examples/resources/casper_contract_schemas/counter_factory_schema.json(2 hunks)examples/resources/casper_contract_schemas/f_token_factory_schema.json(1 hunks)examples/resources/casper_contract_schemas/f_token_schema.json(1 hunks)examples/resources/legacy/better_counter_factory_schema.json(1 hunks)examples/resources/legacy/counter_factory_schema.json(2 hunks)examples/resources/legacy/f_token_factory_schema.json(1 hunks)examples/resources/legacy/f_token_schema.json(1 hunks)examples/src/factory/token.rs(1 hunks)odra-macros/src/ast/factory/impl_item.rs(13 hunks)odra-macros/src/ast/schema/entry_points.rs(2 hunks)odra-macros/src/ir/mod.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/resources/legacy/f_token_factory_schema.json
🧰 Additional context used
🧬 Code graph analysis (4)
examples/src/factory/token.rs (1)
examples/src/factory/counter.rs (3)
init(11-13)init(32-34)test_standalone_module(68-74)
odra-macros/src/ast/schema/entry_points.rs (5)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (2)
vec(78-117)vec(99-102)odra-macros/src/utils/expr.rs (1)
vec(145-147)odra-macros/src/utils/ty.rs (4)
vec(196-198)schema(99-101)string(167-169)entry_point(80-82)odra-macros/src/ir/mod.rs (3)
factory_fn(361-371)factory_upgrade_fn(373-380)factory_batch_upgrade_fn(382-388)odra-schema/src/lib.rs (8)
schema(169-222)schema(456-456)argument(70-76)argument(323-323)argument(330-330)argument(337-337)entry_point(79-94)entry_point(338-338)
odra-macros/src/ir/mod.rs (3)
core/src/host.rs (4)
address(45-47)address(1038-1040)new_contract(385-390)new_contract(543-560)odra-macros/src/utils/ty.rs (4)
address(4-6)uref(72-74)string(167-169)bytes(276-278)odra-vm/src/vm/odra_vm.rs (2)
new_contract(42-63)new(37-39)
odra-macros/src/ast/factory/impl_item.rs (4)
odra-macros/src/utils/ty.rs (8)
string(167-169)from(216-218)vec(196-198)address(4-6)entry_point(80-82)entry_points(76-78)schemas(96-98)schema(99-101)core/src/entry_point.rs (4)
from(60-70)entry_point(73-86)add(50-50)add(54-56)core/src/host.rs (14)
from(134-136)new_contract(385-390)new_contract(543-560)env(33-33)address(45-47)address(1038-1040)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)upgrade_contract(394-400)upgrade_contract(564-588)contract_address(31-31)odra-casper/wasm-env/src/host_functions.rs (7)
override_factory_caller(1072-1076)get_named_arg(325-350)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)upgrade_contract(196-312)
🔇 Additional comments (24)
examples/resources/legacy/f_token_schema.json (1)
1-327: Well-structured schema definition for FToken contract.The schema is correctly formatted and comprehensively defines a standard token contract with:
- Eight events covering token operations (Mint, Burn, Transfer, TransferFrom, SetAllowance, IncreaseAllowance, DecreaseAllowance, OwnershipTransferred)
- Seven public entrypoints including initialization, token transfers, balance queries, and metadata access
- Consistent type and metadata definitions throughout
- Appropriate mutability flags and return types
- Proper use of Option for optional ownership in OwnershipTransferred event
The schema aligns with the factory-upgrade feature objective of introducing a factory token to the system.
examples/resources/legacy/better_counter_factory_schema.json (1)
1-121: LGTM! Schema structure is consistent with the factory upgrade feature.The schema correctly defines the renamed
new_contractentry point and the new upgrade entry points (upgrade_child_contract,batch_upgrade_child_contract) with appropriate argument types and return types.examples/resources/casper_contract_schemas/f_token_schema.json (1)
1-405: LGTM! FToken schema is well-structured.The contract schema properly defines a standard token implementation with events, errors, and entry points. The
odra_cfg_*arguments in the call section align with the framework's deployment configuration pattern.examples/resources/legacy/counter_factory_schema.json (2)
34-60: LGTM! Entry point successfully renamed fromfactorytonew_contract.The renamed entry point maintains the same arguments and return type, ensuring backward compatibility at the API level while adopting the new naming convention.
61-112: LGTM! Upgrade entry points added correctly.The two new entry points (
upgrade_child_contractandbatch_upgrade_child_contract) follow the expected pattern for factory upgrade functionality. The difference in arguments between this schema andbetter_counter_factory_schema.json(which includesnew_value) reflects different upgrader signatures, which is valid.odra-macros/src/ir/mod.rs (5)
22-29: LGTM! FnType enum provides clear function categorization.The public enum enables external code to distinguish between different entry point types, supporting the expanded factory upgrade functionality.
361-371: LGTM! Factory function renamed and signature updated.The
factory_fnmethod now generates anew_contractfunction with aStringtype forcontract_name, aligning with the PR's naming convention changes.
373-388: LGTM! Factory upgrade methods added.Both
factory_upgrade_fnandfactory_batch_upgrade_fncorrectly generate the new upgrade entry points with appropriate argument types (String for contract names, Bytes for serialized arguments).
530-540: LGTM! Function type introspection method added.The
fn_type()method provides a clean way to determine the entry point category based on function name, supporting the new factory upgrade flow.
543-550: LGTM! Protected functions list updated.The array correctly includes the new entry point names (
new_contract,upgrade_child_contract,batch_upgrade_child_contract) to prevent users from declaring functions with these reserved names.examples/resources/casper_contract_schemas/counter_factory_schema.json (2)
32-57: LGTM! Entry point renamed consistently across schema formats.The Casper contract schema correctly reflects the
factory→new_contractrename with proper metadata (is_contract_context: true,access: public).
58-107: LGTM! Upgrade entry points match the legacy schema.Both
upgrade_child_contractandbatch_upgrade_child_contractare properly defined with consistent argument types and access levels.odra-macros/src/ast/schema/entry_points.rs (2)
45-46: LGTM! Factory schema generation updated for all three factory entry points.The chain now includes
factory_fn(),factory_upgrade_fn(), andfactory_batch_upgrade_fn(), ensuring all factory-related entry points are included in the generated schema.
304-329: LGTM! Test expectations updated correctly.The test reflects the renamed
new_contractentry point with the fully-qualifiedodra::prelude::string::Stringtype, and includes both new upgrade entry points with their respective argument types (String for contract names, Bytes for serialized data).examples/resources/casper_contract_schemas/better_counter_factory_schema.json (1)
1-151: LGTM! BetterCounterFactory schema correctly mirrors legacy format.The Casper contract schema properly defines all three entry points with consistent metadata. The
upgrade_child_contractentry point includes thenew_valueargument, distinguishing it from the basic CounterFactory and reflecting the BetterCounter's upgrader signature.examples/resources/casper_contract_schemas/f_token_factory_schema.json (1)
1-163: LGTM! FTokenFactory schema follows the factory pattern correctly.The schema properly defines the factory contract for FToken deployment, with:
new_contractaccepting token-specific initialization parameters (name, symbol, decimals, initial_supply) in addition to contract_nameupgrade_child_contractandbatch_upgrade_child_contractfor upgrade operations- Consistent return types and access levels
odra-macros/src/ast/factory/impl_item.rs (5)
84-115: LGTM: Entry point definitions are well-structured.The refactoring from
factorytonew_contractis consistent, and the two new upgrade-related entry points (upgrade_child_contractandbatch_upgrade_child_contract) follow the same pattern with appropriate mutability and argument types.
150-213: LGTM: ContractRef methods are correctly implemented.All three methods (
new_contract,upgrade_child_contract,batch_upgrade_child_contract) properly construct runtime arguments, handle attached values, and delegate to the contract environment.
505-532: LGTM: Child contract entry points properly separated.The
child_contract_entry_points()function correctly generates entry points for child contracts with appropriate types (Constructorinstead ofTemplate,Regularinstead ofTemplate). This separation from the parent factory's entry points is a good design choice.
753-778: LGTM: Schema entry points are correctly defined.The schema declarations properly reflect the three main entry points (
new_contract,upgrade_child_contract,batch_upgrade_child_contract) with correct return types and argument specifications.
298-364: Configuration flags are correctly aligned.All
odra_cfg_*arguments in thetry_methods match the expectations of their corresponding host functions:
try_new_contractarguments (is_upgradable,is_upgrade,allow_key_override,package_hash_key_name) align withnew_contractrequirements.try_upgrade_child_contractandtry_batch_upgrade_child_contractarguments (is_factory_upgrade,allow_key_override,create_upgrade_group) align withupgrade_contractrequirements.These flags are consistently defined in
core/src/consts.rsand used identically across the codebase.examples/src/factory/token.rs (3)
4-10: Module structure and submodule composition look goodUsing
SubModule<Cep18>plusSubModule<Ownable>in a singleFTokenmodule is a clean way to compose token and ownership behavior, and#[odra::module(factory=on)]on the struct matches the factory example pattern.
48-65: Standalone token test is solid coverage for basic metadata and ownershipThe standalone deployment test validates owner, name, symbol, and total supply, which are the critical invariants for this example. This is a good smoke test for the new factory‑enabled module.
67-86: The review comment is incorrect and should be disregarded.The
HostRef::newsignature at core/src/host.rs:24 is:fn new(address: Address, env: HostEnv) -> Self;It takes ownership of
HostEnv(not a reference). The suggestion to use&envis incorrect—the signature requires an owned value, not a borrow.The current code at line 81:
let token = FTokenHostRef::new(addr, env);is correct. The
envvariable moves into the function call, which is valid becauseenvis not used after this point in the test. The inconsistency with other code sites that useenv.clone()exists only because those contexts reuseenvlater.Likely an incorrect or invalid review comment.
| pub fn init(&mut self, name: String, symbol: String, decimals: u8, initial_supply: U256) { | ||
| self.token.init(symbol, name, decimals, initial_supply); | ||
| let owner = self.env().caller(); | ||
| self.ownable.init(owner); | ||
| } |
There was a problem hiding this comment.
Fix likely swapped name/symbol arguments in init
init takes (name, symbol, decimals, initial_supply) but calls:
self.token.init(symbol, name, decimals, initial_supply);This appears reversed and will likely store symbol as name and vice‑versa, conflicting with expectations in the tests (token.name() == "MyToken", token.symbol() == "MTK").
Recommend:
- self.token.init(symbol, name, decimals, initial_supply);
+ self.token.init(name, symbol, decimals, initial_supply);🤖 Prompt for AI Agents
In examples/src/factory/token.rs around lines 14 to 18, the init call swaps the
name and symbol parameters (method signature is init(name, symbol, decimals,
initial_supply) but code passes symbol first), causing name and symbol to be
stored incorrectly; change the call to pass name then symbol in the same order
as the function signature so the token stores the correct name and symbol.
| ), | ||
| ); | ||
| fn new_contract() { | ||
| odra::odra_casper_wasm_env::host_functions::override_factory_caller(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the necessity of override_factory_caller() in all entry points.
The override_factory_caller() function is called at the beginning of new_contract and several other entry points (lines 554, 692, 698, 709, 715). Please verify:
- Why this override is necessary for these specific entry points
- Whether the override should also be applied to the new
upgrade_child_contractandbatch_upgrade_child_contractentry points (lines 591, 630)
🏁 Script executed:
#!/bin/bash
# Find all uses of override_factory_caller to understand the pattern
rg -n "override_factory_caller" --type rust -A3 -B3Length of output: 8553
🏁 Script executed:
#!/bin/bash
# Check upgrade_child_contract and batch_upgrade_child_contract for override_factory_caller
rg -n "upgrade_child_contract|batch_upgrade_child_contract" --type rust -A10 -B2 odra-macros/src/ast/factory/impl_item.rsLength of output: 11045
🏁 Script executed:
#!/bin/bash
# Search for CALLER_OVERRIDE usage to understand its purpose
rg -n "CALLER_OVERRIDE" --type rust -B3 -A3Length of output: 2217
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about override_factory_caller purpose
rg -n "override_factory_caller|override.*caller" --type rust -B5 | head -100Length of output: 7023
Add override_factory_caller() to both upgrade entry points.
The upgrade_child_contract and batch_upgrade_child_contract entry points (lines 591, 630) are missing the override_factory_caller() call that all other factory entry points have (new_contract, init, total_supply, pay_to_mint, approve). This flag controls caller context resolution in factory contracts—when set, it retrieves the caller from the call stack rather than the immediate caller, which is necessary for proper execution context in factory functions.
Add the call at the start of each function:
- Line 591:
upgrade_child_contract() - Line 630:
batch_upgrade_child_contract()
🤖 Prompt for AI Agents
In odra-macros/src/ast/factory/impl_item.rs around lines 554 through 630, the
upgrade entry points upgrade_child_contract (line ~591) and
batch_upgrade_child_contract (line ~630) are missing the call to
odra::odra_casper_wasm_env::host_functions::override_factory_caller();; add that
call as the first statement in both functions (immediately after the function
prologue/attribute lines and before any other logic) so their caller context
resolves from the call stack like the other factory entry points.
| let contract_key = odra::odra_casper_wasm_env::casper_contract::contract_api::runtime::get_key(&name); | ||
| if let Some(key) = contract_key { | ||
| let package_hash = UnwrapOrRevert::unwrap_or_revert(key.into_package_hash()); | ||
| let mut named_args = specific_args.get(&name).cloned().unwrap_or_else(|| default_args.clone()); | ||
| let _ = named_args.insert("odra_cfg_package_hash_to_upgrade", package_hash.value()); | ||
| let _ = named_args.insert("odra_cfg_package_hash_key_name", name.clone()); | ||
| let contract_package_hash = odra::odra_casper_wasm_env::host_functions::upgrade_contract( | ||
| child_contract_entry_points(), | ||
| schemas.clone(), | ||
| Some(named_args) | ||
| ); | ||
| let address: Address = contract_package_hash.into(); | ||
|
|
||
| exec_env.emit_event(Erc20FactoryContractDeployed { | ||
| contract_name: name, | ||
| contract_address: address | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider handling missing contract keys explicitly.
The batch upgrade silently skips contracts when get_key(&name) returns None (line 670). This could lead to confusion if a user expects all contracts in names_to_upgrade to be upgraded but some are silently skipped because the key doesn't exist.
Consider either:
- Failing with a clear error if any contract key is missing
- Emitting a different event or logging to indicate which contracts were skipped
To fail on missing keys:
for name in names_to_upgrade {
- let contract_key = odra::odra_casper_wasm_env::casper_contract::contract_api::runtime::get_key(&name);
- if let Some(key) = contract_key {
+ let contract_key = UnwrapOrRevert::unwrap_or_revert(
+ odra::odra_casper_wasm_env::casper_contract::contract_api::runtime::get_key(&name)
+ );
+ {
+ let key = contract_key;
let package_hash = UnwrapOrRevert::unwrap_or_revert(key.into_package_hash());
// ... rest of the logic
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In odra-macros/src/ast/factory/impl_item.rs around lines 669 to 686, the code
currently silently skips when get_key(&name) returns None; change this to make
missing contract keys explicit by handling the None branch. Replace the current
if let Some(key) { ... } pattern with an explicit match or an if/else: for the
None case either return/fail with a clear error that includes the contract name
(e.g. panic!/revert/Err with a descriptive message) or emit a distinct event/log
indicating the skipped contract (including the name) before continuing; ensure
the chosen behavior is consistent with surrounding error-handling (propagate an
error if upgrades should abort on missing keys, or emit ExecEnv event/log if
skips are acceptable).
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
odra-macros/src/ast/ref_utils.rs (1)
109-134: Restoreodra_cfg_package_hash_key_namefor factory upgradesThe
FnType::FactoryUpgraderbranch still omits theodra_cfg_package_hash_key_nameruntime arg. Without it the host cannot locate the child package to upgrade, so factory upgrades (and the flows depending on them) will fail at runtime. This regression was pointed out in the earlier review and is still unresolved. Please inject the package-hash key alongside the other config flags.FnType::FactoryUpgrader => syn::parse_quote!(#ty_call_def::new(#fun_name, #is_mut, { let mut #args = #new_runtime_args; #(#fn_args)* #ty_ep_arg::insert_runtime_arg(true, "odra_cfg_is_factory_upgrade", &mut #args); #ty_ep_arg::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut #args); #ty_ep_arg::insert_runtime_arg(false, "odra_cfg_create_upgrade_group", &mut #args); + #ty_ep_arg::insert_runtime_arg(contract_name.clone(), "odra_cfg_package_hash_key_name", &mut #args); #args })),odra-macros/src/ir/mod.rs (1)
415-422: LGTM! Visibility issue resolved.The
upgrader_args()method is now correctly private, matching theconstructor_args()pattern. This addresses the concern from the previous review.odra-macros/src/ast/factory/impl_item.rs (3)
666-702: Addoverride_factory_caller()to upgrade entry point.The
upgrade_child_contractentry point (line 666) is missing theoverride_factory_caller()call that other factory entry points have (lines 518, 631, 637, 648, 654). This flag controls caller context resolution—when set, it retrieves the caller from the call stack rather than the immediate caller, which is necessary for proper execution context in factory functions.Add the call at the start of the function:
#[no_mangle] fn upgrade_child_contract() { + odra::odra_casper_wasm_env::host_functions::override_factory_caller(); use odra::odra_casper_wasm_env::casper_contract::unwrap_or_revert::UnwrapOrRevert;
705-738: Addoverride_factory_caller()to batch upgrade entry point.The
batch_upgrade_child_contractentry point (line 705) is also missing theoverride_factory_caller()call. This should be consistent with other factory entry points for proper caller context resolution.Add the call at the start of the function:
#[no_mangle] fn batch_upgrade_child_contract() { + odra::odra_casper_wasm_env::host_functions::override_factory_caller(); use odra::odra_casper_wasm_env::casper_contract::unwrap_or_revert::UnwrapOrRevert;
718-737: Consider handling missing contract keys explicitly.The batch upgrade loop (lines 718-737) silently skips contracts when
get_key(&name)returnsNone(line 720). This could confuse users who expect all contracts in the batch to be upgraded but some are silently skipped because keys don't exist.Consider either:
- Failing with a clear error if any contract key is missing
- Emitting a distinct event or log indicating which contracts were skipped
odra-macros/src/ast/factory/parts/wasm_parts.rs (1)
396-515: Hardcoded child contract type limits factory flexibility.The macro-generated code hardcodes the child contract's event schema type (e.g.,
<Erc20 as ...>in the test at lines 632, 669, 709). This creates tight coupling where factory contracts can only upgrade one specific child contract type, fundamentally limiting the factory pattern's purpose of managing multiple contract types.The
NoMangleFactoryUpgradeFnItem::try_from()implementation (lines 402-421) extracts the module identifier and derives the child contract type by stripping "Factory" suffix (lines 432-434 in ToTokens). However, this assumes:
- All child contracts share the same event schema type
- The factory only manages one contract type
- The naming convention (Factory suffix) is always followed
Consider refactoring to:
- Pass the child contract type information as a runtime parameter or configuration
- Use a trait-based approach where child contracts must implement a common interface
- Store contract type metadata when contracts are deployed so upgrades can look it up dynamically
This is a significant architectural limitation that should be addressed before the factory upgrade feature is widely used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/src/args.rs(1 hunks)core/src/entry_point.rs(1 hunks)examples/bin/factory_example.rs(1 hunks)examples/src/factory/counter.rs(3 hunks)odra-macros/src/ast/contract_ref_item.rs(5 hunks)odra-macros/src/ast/external_contract_item.rs(2 hunks)odra-macros/src/ast/factory/impl_item.rs(11 hunks)odra-macros/src/ast/factory/parts/has_entrypoints_item.rs(3 hunks)odra-macros/src/ast/factory/parts/ref_item.rs(2 hunks)odra-macros/src/ast/factory/parts/test_parts.rs(3 hunks)odra-macros/src/ast/factory/parts/wasm_parts.rs(23 hunks)odra-macros/src/ast/host_ref_item.rs(5 hunks)odra-macros/src/ast/ref_utils.rs(5 hunks)odra-macros/src/ast/schema/entry_points.rs(4 hunks)odra-macros/src/ast/test_parts.rs(4 hunks)odra-macros/src/ast/wasm_parts.rs(11 hunks)odra-macros/src/ir/mod.rs(5 hunks)odra-macros/src/utils/expr.rs(2 hunks)odra-macros/src/utils/ty.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- odra-macros/src/utils/ty.rs
- odra-macros/src/ast/factory/parts/ref_item.rs
🧰 Additional context used
🧬 Code graph analysis (17)
odra-macros/src/ast/host_ref_item.rs (2)
core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)odra-macros/tests/test.rs (2)
insert_runtime_arg(8-8)insert_runtime_arg(12-14)
odra-macros/src/ast/external_contract_item.rs (1)
core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)
odra-macros/src/ast/schema/entry_points.rs (3)
odra-macros/src/ir/mod.rs (4)
ty(791-796)factory_fn(362-372)factory_upgrade_fn(374-381)factory_batch_upgrade_fn(383-390)odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (1)
vec(77-99)odra-macros/src/utils/ty.rs (3)
vec(196-198)bytes(276-278)string(167-169)
examples/bin/factory_example.rs (3)
examples/src/factory/counter.rs (3)
InstallConfig(103-103)InstallConfig(146-146)InstallConfig(191-191)core/src/entry_point.rs (2)
factory(206-215)from(67-79)core/src/args.rs (1)
from(149-151)
core/src/args.rs (2)
core/src/entry_point_callback.rs (3)
new(38-48)new(80-86)new(109-114)odra-macros/tests/test.rs (2)
insert_runtime_arg(8-8)insert_runtime_arg(12-14)
examples/src/factory/counter.rs (4)
odra-macros/src/ast/factory/parts/wasm_parts.rs (4)
module(74-79)module(161-165)module(166-170)module(408-413)core/src/entry_point.rs (2)
factory(206-215)from(67-79)core/src/host.rs (18)
InstallConfig(187-187)InstallConfig(227-227)env(33-33)deploy(80-80)deploy(206-218)address(45-47)address(1038-1040)from(134-136)deploy_with_cfg(90-90)deploy_with_cfg(231-244)upgradable(186-188)try_upgrade(102-106)try_upgrade(267-278)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)core/src/args.rs (1)
from(149-151)
odra-macros/src/ast/ref_utils.rs (5)
odra-macros/src/ir/mod.rs (12)
ty(791-796)name(604-606)name(777-785)syn(205-205)syn(206-206)syn(210-210)syn(472-472)syn(825-825)new(725-727)new(743-745)new(769-771)generics(662-664)odra-macros/src/utils/ty.rs (1)
entry_point_arg(107-109)core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)odra-macros/src/utils/expr.rs (2)
args(177-180)new_runtime_args(4-7)odra-macros/src/ast/fn_utils.rs (4)
fun(27-30)new(48-64)new(85-89)insert_args_stmts(23-31)
odra-macros/src/ast/contract_ref_item.rs (2)
core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)odra-macros/tests/test.rs (2)
insert_runtime_arg(8-8)insert_runtime_arg(12-14)
odra-macros/src/utils/expr.rs (2)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (1)
vec(77-99)core/src/args.rs (1)
unwrap_or_default(50-55)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (3)
odra-macros/src/utils/expr.rs (9)
vec(145-147)args(177-180)constructor_ep(209-213)upgrader_ep(215-219)factory_ep(237-241)factory_upgrade_ep(243-247)factory_batch_upgrade_ep(249-251)regular_ep(221-235)vec_try_into(149-155)odra-macros/src/ir/mod.rs (8)
factory_fn(362-372)factory_upgrade_fn(374-381)factory_batch_upgrade_fn(383-390)syn(205-205)syn(206-206)syn(210-210)syn(472-472)syn(825-825)odra-macros/src/ast/wasm_parts_utils.rs (2)
param_parameters(7-16)param_ret_ty(18-23)
odra-macros/src/ast/factory/parts/test_parts.rs (3)
odra-casper/test-vm/src/vm/casper_vm.rs (5)
new_contract(440-464)value(815-820)value(827-832)call_contract(308-378)new(75-77)core/src/args.rs (10)
unwrap(32-37)unwrap(98-98)unwrap(116-121)unwrap(137-142)unwrap(176-181)from(149-151)insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)odra-casper/wasm-env/src/host_functions.rs (1)
call_contract(574-595)
odra-macros/src/ast/test_parts.rs (1)
core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)
odra-macros/src/ir/mod.rs (3)
odra-macros/src/utils/ty.rs (4)
address(4-6)string(167-169)runtime_args(123-125)batch_upgrade_args_of_t(280-282)core/src/host.rs (4)
address(45-47)address(1038-1040)new_contract(385-390)new_contract(543-560)odra-vm/src/vm/odra_vm.rs (2)
new_contract(42-63)new(37-39)
core/src/entry_point.rs (4)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (1)
vec(77-99)odra-macros/src/utils/expr.rs (2)
vec(145-147)args(177-180)odra-macros/src/utils/ty.rs (3)
vec(196-198)from(216-218)entry_point(80-82)odra-macros/src/ast/factory/parts/wasm_parts.rs (11)
try_from(18-45)try_from(63-81)try_from(159-175)try_from(212-219)try_from(225-238)try_from(244-250)try_from(256-263)try_from(269-274)try_from(314-318)try_from(330-339)try_from(405-420)
odra-macros/src/ast/factory/impl_item.rs (5)
core/src/entry_point.rs (4)
entry_point(165-178)from(67-79)add(57-57)add(61-63)core/src/args.rs (21)
parameter(186-191)parameter(252-260)parameter(253-253)parameter(254-254)parameter(255-255)parameter(256-256)unwrap_or_default(50-55)from(149-151)insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(163-174)unwrap(32-37)unwrap(98-98)unwrap(116-121)unwrap(137-142)unwrap(176-181)cl_type(94-94)cl_type(106-108)cl_type(129-131)cl_type(159-161)odra-casper/test-vm/src/vm/casper_vm.rs (4)
new_contract(440-464)call_contract(308-378)new(75-77)upgrade_contract(467-484)core/src/host.rs (16)
new_contract(385-390)new_contract(543-560)env(33-33)call_contract(377-382)call_contract(606-618)address(45-47)address(1038-1040)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)from(134-136)upgrade_contract(394-400)upgrade_contract(564-588)contract_address(31-31)odra-casper/wasm-env/src/host_functions.rs (8)
call_contract(574-595)override_factory_caller(1072-1076)get_named_arg(325-350)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)upgrade_contract(196-312)
odra-macros/src/ast/factory/parts/wasm_parts.rs (6)
odra-macros/src/ir/mod.rs (21)
syn(205-205)syn(206-206)syn(210-210)syn(472-472)syn(825-825)try_from(202-218)try_from(578-582)try_from(588-592)try_from(823-827)host_functions(351-353)factory_upgrade_fn(374-381)ty(791-796)upgrader_args(415-422)name(604-606)name(777-785)named_args(638-643)new(725-727)new(743-745)new(769-771)contract_name(138-144)is_payable(620-623)core/src/entry_point.rs (2)
try_from(85-149)entry_point(165-178)odra-macros/src/ast/wasm_parts_utils.rs (4)
func(8-14)use_entity_entry_points_ext(35-37)param_parameters(7-16)param_ret_ty(18-23)odra-macros/src/utils/ident.rs (8)
ident(86-88)entry_points(47-49)child_contract_entry_points(51-53)schemas(59-61)event_schemas(74-76)named_args(3-5)env(19-21)cl_type(106-108)odra-macros/src/utils/expr.rs (12)
new_wasm_contract_env(73-75)args(177-180)template_ep(253-259)constructor_ep(209-213)upgrader_ep(215-219)regular_ep(221-235)factory_ep(237-241)factory_upgrade_ep(243-247)factory_batch_upgrade_ep(249-251)schemas(68-71)event_schemas(86-89)vec(145-147)odra-casper/wasm-env/src/host_functions.rs (1)
override_factory_caller(1072-1076)
odra-macros/src/ast/wasm_parts.rs (4)
odra-macros/src/ast/wasm_parts_utils.rs (4)
use_entity_entry_points_ext(35-37)func(8-14)param_parameters(7-16)param_ret_ty(18-23)odra-macros/src/utils/attr.rs (1)
no_mangle(15-17)odra-macros/src/utils/expr.rs (5)
args(177-180)constructor_ep(209-213)upgrader_ep(215-219)regular_ep(221-235)vec(145-147)odra-macros/src/utils/ident.rs (4)
ident(86-88)entry_points(47-49)add_entry_point(55-57)cl_type(106-108)
🔇 Additional comments (1)
odra-macros/src/ast/external_contract_item.rs (1)
87-87: LGTM! Unnecessary clone operations removed.The changes correctly remove
.clone()calls when passingownertoinsert_runtime_arg. Since the trait method consumesselfby value andowneris not used after these calls, moving the value directly is both safe and more efficient than cloning.Also applies to: 172-172
Benchmark report
|
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/src/factory/token.rs (1)
14-18: Critical: Swappedname/symbolarguments still not fixed from previous review.This critical issue was flagged in the previous review but remains unaddressed. Line 15 still swaps the
nameandsymbolparameters when callingself.token.init():self.token.init(symbol, name, decimals, initial_supply);This will cause:
token.name()to return the symbol valuetoken.symbol()to return the name valueThe tests on lines 62-63 and 83-84 expect the correct mapping and will fail with this bug.
Apply this diff to fix the parameter order:
- self.token.init(symbol, name, decimals, initial_supply); + self.token.init(name, symbol, decimals, initial_supply);examples/src/factory/counter.rs (1)
24-47: Clarify/guard access toBetterCounter::upgradeentry point
upgradeis a public entry point that directly overwritesvalue. The new tests enforce authorization at the factory level, but ifBetterCounteris ever called directly (e.g., via its host ref), callers could bypass factory-level checks unless the framework guarantees this method is only invocable during an upgrade context. Consider either:
- Adding an explicit context/ownership check inside
upgrade, or- Documenting that this entry point is only safe to call via the factory’s upgrade APIs.
🧹 Nitpick comments (2)
examples/src/factory/counter.rs (2)
95-136: Happy‑path factory upgrade test is thorough; consider splitting responsibilities
test_factory_upgrade_worksnicely covers: upgrading the factory toBetterCounterFactory, single child upgrades, and batch upgrades, then verifying all resulting values. For easier debugging when something breaks, you might optionally split this into two tests (single‑upgrade vs batch‑upgrade), but the current structure is functionally sound.
138-181: Unauthorized‑caller test correctly assertsInvalidContext; tighten batch args if desiredThe test clearly validates that both single and batch child upgrades fail with
VmError::InvalidContextafter switching the caller account, and checks the child value remains unchanged. For extra future‑proofing, you could align the batch upgrade map keys with actually deployed child names, so the failure reason remains purely about context even if the implementation later changes validation order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/src/factory/counter.rs(3 hunks)examples/src/factory/token.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/src/factory/token.rs (2)
odra-macros/src/ast/factory/parts/wasm_parts.rs (5)
module(74-79)module(161-165)module(166-170)module(408-413)test(523-772)examples/src/factory/counter.rs (3)
init(11-13)init(32-34)test_standalone_module(64-70)
examples/src/factory/counter.rs (3)
core/src/entry_point.rs (2)
factory(206-215)from(67-79)core/src/host.rs (14)
InstallConfig(187-187)InstallConfig(227-227)from(134-136)env(33-33)deploy_with_cfg(90-90)deploy_with_cfg(231-244)upgradable(186-188)try_upgrade(102-106)try_upgrade(267-278)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)core/src/args.rs (1)
from(149-151)
🔇 Additional comments (7)
examples/src/factory/token.rs (4)
1-10: LGTM! Doc comment fixed.The module structure and factory configuration are correct. The doc comment on line 6 has been properly updated from the previous review feedback.
20-33: LGTM!The delegate block correctly forwards methods to the appropriate submodules (
tokenandownable).
48-65: Test logic is correct.The standalone deployment test correctly validates owner, name, symbol, and total supply. Note that this test will fail until the parameter swap bug on line 15 is fixed.
67-86: Factory test logic is correct.The factory deployment test correctly validates the factory workflow and token properties. The test is appropriately marked as ignored for odra vm. Note that this test will also fail until the parameter swap bug on line 15 is fixed.
examples/src/factory/counter.rs (3)
52-58: Imports and BetterCounter factory wiring look consistentThe added imports (
InstallConfig,NoArgs,VmError,BetterCounterFactory,BetterCounterUpgradeArgs) and their later uses align with the new typed deploy/upgrade and batch-upgrade APIs, with no obvious redundancy or mismatch.
79-79: Use ofnew_contractmatches updated factory entry pointSwitching to
factory_ref.new_contract(String::from("Counter"), 10)(name first, then init arg) correctly reflects the newnew_contractentry point signature and keeps the example in sync with the core factory API.
183-213: Invalid‑arg batch upgrade test correctly targetsMissingArgbehavior
test_factory_upgrade_fails_for_invalid_argsets up valid children, upgrades the factory, then intentionally passesNoArgswhereBetterCounterUpgradeArgsis expected and assertsExecutionError::MissingArg. This accurately captures the typed batch‑upgrade argument validation behavior and gives good regression coverage.
…y FactoryBatchUpgrade entry point documentation
Benchmark report
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
core/src/entry_point.rs (1)
45-46: Fix duplicate documentation comment.The documentation for
FactoryBatchUpgradeis identical toFactoryUpgrade(line 40-41). The comment should describe batch upgrade functionality specifically.Apply this diff:
- /// A factory upgrade entry point. + /// A factory batch upgrade entry point. FactoryBatchUpgrade,odra-macros/src/ir/mod.rs (1)
420-427: Makeupgrader_args()private to matchconstructor_args().Both
constructor_args()(line 411) andupgrader_args()(line 420) follow identical patterns and are only used internally.constructor_args()is private whileupgrader_args()is public. This visibility inconsistency is unnecessary.Apply this diff:
- pub fn upgrader_args(&self) -> syn::punctuated::Punctuated<syn::FnArg, syn::token::Comma> { + fn upgrader_args(&self) -> syn::punctuated::Punctuated<syn::FnArg, syn::token::Comma> { self.upgrader() .map(|fn_ir| fn_ir.named_args()) .unwrap_or_default() .iter() .map(FnArgIR::raw) .collect::<syn::punctuated::Punctuated<syn::FnArg, syn::token::Comma>>() }odra-macros/src/ast/factory/parts/ref_item.rs (1)
111-168: Verify runtime configuration flags for upgrade operations.The
upgrade_child_contract(lines 111-136) andbatch_upgrade_child_contract(lines 138-168) methods don't appear to setodra_cfg_*runtime arguments in this test expectation. Based on past review comments, upgrade operations intest_parts.rsset configuration flags like:
odra_cfg_is_factory_upgrade = trueodra_cfg_allow_key_override = trueodra_cfg_create_upgrade_group = falseIf these flags are required for upgrade operations, they should be included in the ref_item test expectations as well to maintain consistency.
Note: The past review indicated this was "Addressed in commit 3a056e3". Verify whether these test expectations reflect the latest implementation or if they need updating.
#!/bin/bash # Compare upgrade_child_contract implementations between test_parts and ref_item rg -A 20 "upgrade_child_contract" odra-macros/src/ast/factory/parts/test_parts.rs | head -30odra-macros/src/ast/factory/impl_item.rs (1)
239-361: Upgrade WASM entrypoints still lackoverride_factory_caller()compared to other factory entrypointsIn the generated WASM module,
new_contract,init,total_supply,pay_to_mint, andapproveall invokeodra::odra_casper_wasm_env::host_functions::override_factory_caller()at the start, but the newupgrade_child_contractandbatch_upgrade_child_contractentrypoints do not.Given the earlier review already highlighted this for the same logical code path, and
override_factory_caller()controls how the caller is resolved for factory operations, please confirm whether upgrades should share that behavior. If yes, the upgrade entrypoints should also calloverride_factory_caller()immediately after function prologue to keep caller semantics consistent withnew_contract.Also applies to: 416-552, 554-627
odra-macros/src/ast/factory/parts/wasm_parts.rs (1)
396-515: Upgrade wasm entrypoints still missingoverride_factory_caller()despite factory contextThe generated
upgrade_child_contractandbatch_upgrade_child_contractwasm functions correctly:
- resolve the child contract package hash,
- inject
odra_cfg_package_hash_to_upgradeandodra_cfg_package_hash_key_name,- delegate to
host_functions::upgrade_contractwithchild_contract_entry_points(), and- emit the deployment event with the upgraded address.
However, unlike
new_contract(and other factory-context entrypoints), they do not callodra::odra_casper_wasm_env::host_functions::override_factory_caller()before constructing theExecutionEnv. If upgrades are meant to share the same “logical caller is the factory client, not the factory contract” semantics, these entrypoints should also invoke the override at the top.This mirrors the earlier review comment on the same behavior in the generated Erc20-specific code; please confirm the intended caller semantics and align accordingly.
🧹 Nitpick comments (2)
odra-macros/src/utils/expr.rs (1)
149-155: Document silent failure behavior invec_try_into.The
vec_try_intohelper usesunwrap_or_default()to silently return an empty vector if any conversion fails. This masks conversion errors, which might make debugging difficult. Consider if this is the intended behavior or if errors should be propagated.If error propagation is needed, consider this alternative:
pub fn vec_try_into<T: ToTokens>(content: T) -> syn::Expr { parse_quote!(odra::prelude::vec![#content] .into_iter() .map(TryInto::try_into) .collect::<Result<Vec<_>, _>>()?) }Otherwise, add a comment explaining why silent failure is acceptable in this context.
odra-macros/src/ast/factory/parts/wasm_parts.rs (1)
113-175: Entry point construction via context-specificAddEntryPointStmtItemis sound but slightly redundantThe refactor to
AddEntryPointStmtItem<Ctx>that stores anentry_point_exprand emitsentry_points.add(...)simplifies codegen and cleanly separates:
- template entrypoints for factory functions,
- installer/child entrypoints (constructor/upgrader/regular), and
- the factory / upgrade / batch-upgrade variants.
The logic in the various
TryFromimpls appears consistent with the IR helpers (factory_fn,factory_upgrade_fn,upgrader, etc.), and the test expectations validate the generated shapes. You do, however, iteratemodule.functions()?twice to builditemsandinstaller_items; if this ever shows up hot in compile times, you could consider sharing the collection or caching it in the IR.Also applies to: 193-275
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/args.rs(1 hunks)core/src/entry_point.rs(1 hunks)odra-macros/src/ast/contract_ref_item.rs(5 hunks)odra-macros/src/ast/external_contract_item.rs(2 hunks)odra-macros/src/ast/factory/impl_item.rs(11 hunks)odra-macros/src/ast/factory/parts/ref_item.rs(2 hunks)odra-macros/src/ast/factory/parts/test_parts.rs(3 hunks)odra-macros/src/ast/factory/parts/wasm_parts.rs(23 hunks)odra-macros/src/ast/host_ref_item.rs(5 hunks)odra-macros/src/ast/test_parts.rs(4 hunks)odra-macros/src/ast/wasm_parts.rs(11 hunks)odra-macros/src/ir/mod.rs(5 hunks)odra-macros/src/utils/expr.rs(2 hunks)odra-macros/src/utils/ty.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- odra-macros/src/ast/contract_ref_item.rs
- core/src/args.rs
- odra-macros/src/ast/external_contract_item.rs
🧰 Additional context used
🧬 Code graph analysis (11)
odra-macros/src/utils/ty.rs (2)
odra-macros/src/utils/ident.rs (1)
bytes(90-92)core/src/call_result.rs (1)
bytes(45-52)
odra-macros/src/ast/host_ref_item.rs (1)
core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(166-177)
core/src/entry_point.rs (3)
odra-macros/src/utils/expr.rs (2)
vec(145-147)args(177-180)odra-macros/src/utils/ty.rs (2)
vec(196-198)entry_point(80-82)odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (1)
vec(77-99)
odra-macros/src/utils/expr.rs (3)
odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (1)
vec(77-99)core/src/args.rs (1)
unwrap_or_default(50-55)core/src/entry_point.rs (1)
entry_point(165-178)
odra-macros/src/ast/test_parts.rs (2)
core/src/args.rs (4)
insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(166-177)odra-macros/tests/test.rs (2)
insert_runtime_arg(8-8)insert_runtime_arg(12-14)
odra-macros/src/ast/factory/parts/test_parts.rs (6)
odra-casper/test-vm/src/vm/casper_vm.rs (5)
new_contract(440-464)value(815-820)value(827-832)call_contract(308-378)new(75-77)odra-macros/src/utils/ty.rs (6)
string(167-169)address(4-6)from(216-218)vec(196-198)call_def(44-46)entry_point(80-82)core/src/args.rs (10)
unwrap(32-37)unwrap(98-98)unwrap(116-121)unwrap(137-142)unwrap(179-184)from(149-151)insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(166-177)odra-casper/wasm-env/src/host_functions.rs (1)
call_contract(574-595)core/src/entry_point.rs (2)
from(67-79)entry_point(165-178)odra-macros/src/ast/factory/parts/has_entrypoints_item.rs (1)
vec(77-99)
odra-macros/src/ast/factory/parts/ref_item.rs (3)
odra-macros/src/ir/mod.rs (13)
syn(205-205)syn(206-206)syn(210-210)syn(213-213)syn(477-477)syn(830-830)factory_fn(367-377)factory_upgrade_fn(379-386)factory_batch_upgrade_fn(388-395)contract_name(138-144)new(730-732)new(748-750)new(774-776)odra-macros/src/ast/ref_utils.rs (2)
fun(99-107)contract_function_item(48-58)core/src/args.rs (5)
from(149-151)insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(166-177)
odra-macros/src/ir/mod.rs (3)
odra-macros/src/utils/ty.rs (5)
address(4-6)uref(72-74)string(167-169)runtime_args(123-125)batch_upgrade_args_of_t(280-282)core/src/host.rs (4)
address(45-47)address(1038-1040)new_contract(385-390)new_contract(543-560)odra-vm/src/vm/odra_vm.rs (2)
new_contract(42-63)new(37-39)
odra-macros/src/ast/wasm_parts.rs (5)
odra-macros/src/ast/wasm_parts_utils.rs (4)
use_entity_entry_points_ext(35-37)func(8-14)param_parameters(7-16)param_ret_ty(18-23)odra-macros/src/ast/factory/parts/wasm_parts.rs (6)
to_tokens(85-110)to_tokens(122-153)to_tokens(199-206)to_tokens(282-308)to_tokens(343-393)to_tokens(424-514)odra-macros/src/utils/attr.rs (1)
no_mangle(15-17)odra-macros/src/utils/expr.rs (5)
args(177-180)constructor_ep(209-213)upgrader_ep(215-219)regular_ep(221-235)vec(145-147)odra-macros/src/utils/ident.rs (4)
ident(86-88)entry_points(47-49)add_entry_point(55-57)cl_type(106-108)
odra-macros/src/ast/factory/impl_item.rs (5)
core/src/entry_point.rs (4)
entry_point(165-178)from(67-79)add(57-57)add(61-63)core/src/args.rs (20)
parameter(189-194)parameter(255-263)parameter(256-256)parameter(257-257)parameter(258-258)parameter(259-259)from(149-151)insert_runtime_arg(96-96)insert_runtime_arg(110-114)insert_runtime_arg(133-135)insert_runtime_arg(166-177)unwrap(32-37)unwrap(98-98)unwrap(116-121)unwrap(137-142)unwrap(179-184)cl_type(94-94)cl_type(106-108)cl_type(129-131)cl_type(159-164)odra-casper/test-vm/src/vm/casper_vm.rs (4)
new_contract(440-464)call_contract(308-378)new(75-77)upgrade_contract(467-484)core/src/host.rs (16)
new_contract(385-390)new_contract(543-560)env(33-33)call_contract(377-382)call_contract(606-618)address(45-47)address(1038-1040)new(24-24)new(177-183)new(195-201)new(443-450)new(1035-1037)from(134-136)upgrade_contract(394-400)upgrade_contract(564-588)contract_address(31-31)odra-casper/wasm-env/src/host_functions.rs (8)
call_contract(574-595)override_factory_caller(1072-1076)get_named_arg(325-350)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)upgrade_contract(196-312)
odra-macros/src/ast/factory/parts/wasm_parts.rs (7)
odra-macros/src/ir/mod.rs (23)
syn(205-205)syn(206-206)syn(210-210)syn(213-213)syn(477-477)syn(830-830)try_from(202-223)try_from(583-587)try_from(593-597)try_from(828-832)host_functions(356-358)factory_upgrade_fn(379-386)ty(796-801)upgrader_args(420-427)name(609-611)name(782-790)named_args(643-648)new(730-732)new(748-750)new(774-776)contract_name(138-144)is_non_reentrant(630-633)is_payable(625-628)core/src/entry_point.rs (2)
try_from(85-149)entry_point(165-178)odra-macros/src/ast/wasm_parts.rs (8)
try_from(40-58)try_from(81-99)try_from(123-184)try_from(221-248)try_from(266-283)module(51-56)module(92-96)std(246-246)odra-macros/src/ast/wasm_parts_utils.rs (4)
func(8-14)use_entity_entry_points_ext(35-37)param_parameters(7-16)param_ret_ty(18-23)odra-macros/src/utils/ident.rs (9)
ident(86-88)result(11-13)entry_points(47-49)child_contract_entry_points(51-53)schemas(59-61)event_schemas(74-76)named_args(3-5)env(19-21)cl_type(106-108)odra-macros/src/utils/expr.rs (12)
new_wasm_contract_env(73-75)new_entry_points(30-33)args(177-180)constructor_ep(209-213)upgrader_ep(215-219)regular_ep(221-235)factory_ep(237-241)factory_upgrade_ep(243-247)factory_batch_upgrade_ep(249-251)schemas(68-71)event_schemas(86-89)vec(145-147)odra-casper/wasm-env/src/host_functions.rs (5)
override_factory_caller(1072-1076)runtime(232-232)runtime(233-233)runtime(966-966)runtime(980-980)
🔇 Additional comments (19)
odra-macros/src/utils/ty.rs (1)
276-282: LGTM! New type helpers align with the factory upgrade feature.The addition of
bytes()andbatch_upgrade_args_of_t()provides clean type references for the new batch upgrade functionality. These helpers follow the existing pattern and integrate well with the broader PR changes.odra-macros/src/ast/wasm_parts.rs (2)
187-198: Good refactoring with generic context types.The introduction of
NoMangleItemContexttrait and the genericNoMangleFnItem<C>is a clean abstraction that allows different contexts (likeModuleContext) to be handled systematically. This improves extensibility for future contexts.
68-91: EntityEntryPointsExt is properly exported and accessible.The trait is defined as
pub trait EntityEntryPointsExtin core/src/entry_point.rs and re-exported through the odra crate viapub use odra_core::{..., entry_point, ...}in odra/src/lib.rs. The import pathodra::entry_point::EntityEntryPointsExtresolves correctly.odra-macros/src/ast/host_ref_item.rs (1)
415-526: LGTM! Test expectations align with argument passing changes.The test fixtures correctly reflect the updated
insert_runtime_argsignature that takes arguments by value rather than by reference. This aligns with theEntrypointArgumenttrait implementation shown in the relevant snippets.odra-macros/src/ast/test_parts.rs (1)
177-288: LGTM! Consistent test expectations for argument handling.The test fixtures properly reflect the updated
EntrypointArgument::insert_runtime_argAPI that consumes arguments by value. This is consistent with the changes across other test files.odra-macros/src/ast/factory/parts/ref_item.rs (1)
22-36: Clean refactoring to support multiple factory methods.The change from a single
factory_fnto a collectionfactory_fns: Vec<syn::ItemFn>is a good design that cleanly accommodates the new upgrade methods. The implementation properly builds all three factory-related functions and emits them in theToTokensimplementation.odra-macros/src/ir/mod.rs (1)
388-395: Generic constraints for batch upgrade function are correctly implemented.The verification confirms that
BatchUpgradeArgs<T>properly enforces its generic constraintT: Into<casper_types::RuntimeArgs>in its struct definition. TheFrom<BTreeMap<String, T>>implementation at core/src/args.rs:148-151 provides theInto<BatchUpgradeArgs<T>>conversion needed by the factory method. The constraint chainS: Into<BatchUpgradeArgs<T>>, T: Into<RuntimeArgs>is sound and properly expressed.odra-macros/src/utils/expr.rs (1)
221-235: No issues found—field mapping is correct.The verification confirms:
- The
is_non_reentrantfield name matches the definition in core/src/entry_point.rs:17- Semantics are correctly aligned: when
true, the entry point is marked as non-reentrant (no inversion required)- The
regular_epfunction correctly passes the parameter directly toEntryPoint::Regular- The conversion logic in core/src/entry_point.rs:93–96 properly interprets the boolean value, adding
EntrypointAttribute::NonReentrantwhenis_non_reentrantistruecore/src/entry_point.rs (1)
82-150: Now let me search for actual usages in the codebase:Based on my analysis, the original review comment requests verification of error handling for the TryFrom conversion. The comment notes that Template and Upgrader variants return an error. However, after searching the codebase and examining the usage patterns, I cannot definitively find where this conversion is actually used. Without being able to locate the actual call sites, I cannot verify:
- Whether Template and Upgrader variants are ever instantiated
- How callers handle the error cases
- Whether the error handling is adequate
No actionable concerns identified for this code review.
The TryFrom implementation appropriately returns an error for unsupported EntryPoint variants (Template and Upgrader). The error handling pattern itself is standard in Rust—TryFrom/TryInto traits are used for fallible conversions and return Results. Callers of this conversion are responsible for handling the Result type through standard patterns (match, ?, unwrap, etc.). Without evidence that these unsupported variants are instantiated in the codebase or that error handling at call sites is deficient, there are no specific fixes required.
Likely an incorrect or invalid review comment.
odra-macros/src/ast/factory/parts/test_parts.rs (4)
118-135: HostRef factory wrappers andtry_new_contractargument wiring look consistentThe new
new_contract,upgrade_child_contract, andbatch_upgrade_child_contractwrappers simply delegate to thetry_*variants and unwrap the result, which is in line with other HostRef ergonomics. Intry_new_contract, theodra_cfg_*flags pluscontract_name/valueare assembled coherently and match the expected factory configuration and VM side expectations.Also applies to: 140-159
161-209: Upgrade HostReftry_*paths correctly set factory-upgrade flags
try_upgrade_child_contractandtry_batch_upgrade_child_contractboth:
- forward through
env.call_contractwith the correct entry point names, and- set
odra_cfg_is_factory_upgrade,odra_cfg_allow_key_override, andodra_cfg_create_upgrade_groupflags, plus child-specific args (contract_nameor serializedargs).This aligns with the factory-upgrade contract flow and the
upgrade_contracthost function’s expectations.
218-243: EntryPointsCaller wiring fornew_contractmatches new factory APIThe
EntryPointsCallernow exposes only thenew_contractentry point with thecontract_name: odra::prelude::string::Stringandvalue: u32arguments, and the callback explicitly special-cases"new_contract"with a clear error. That matches the new typed entry point API and keeps the “factory not supported” behavior explicit.
691-715: Schema entrypoints for factory and upgrade flows are coherent with coreEntryPointmappingThe added schema entries for
new_contract,upgrade_child_contract, andbatch_upgrade_child_contractuse:
- return types
(Address, URef)/()as expected, and- argument shapes consistent with the core
EntryPoint→schema conversion (contract_nameasodra::prelude::string::String,argsasbytesrepr::Bytesfor the batch case).This keeps on-chain metadata aligned with the new typed entry point model.
odra-macros/src/ast/factory/impl_item.rs (3)
72-87: Typed factory entrypoints definition aligns with new coreEntryPointmodelThe
HasEntrypointsimpl now advertises:
Constructorwith empty args,Factorywithcontract_name: odra::prelude::string::Stringandvalue: u32,FactoryUpgradewithcontract_name, andFactoryBatchUpgradewith no explicit args,then converts via
TryInto. This matches the coreEntryPointenum and the intendednew_contract/upgrade_child_contract/batch_upgrade_child_contractsurface.
119-179: ContractRef factory API correctly forwards args and attached valueThe new
Erc20FactoryContractRefmethods:
- use
"new_contract","upgrade_child_contract", and"batch_upgrade_child_contract"entry point names,- preserve
attached_valueboth as a named arg"amount"and via.with_amount(self.attached_value), and- assemble
contract_name,value, or serializedargsviaEntrypointArgument::insert_runtime_arg.This is consistent with existing call patterns for payable entry points and the new factory/upgrade entrypoint set.
691-715: Schema entrypoints for factory and upgrade flows are consistent with wasm & core mappingsThe schema for
new_contract,upgrade_child_contract, andbatch_upgrade_child_contractmirrors the expected signatures:
new_contract→(Address, URef)+contract_name/value,upgrade_child_contract→()+contract_name,batch_upgrade_child_contract→()+ singleargs: Bytesargument,which lines up with the
EntryPoint::Factory*variants and the wasm parts usingBTreeMap<String, Bytes>serialized into a single bytes argument.odra-macros/src/ast/factory/parts/wasm_parts.rs (3)
15-46: Factory-specificNoMangleFnItemcorrectly wraps exec parts with caller overrideThe
TryFrom<(&ModuleImplIR, &FnIR)> for NoMangleFnItem<FactoryContext>builds the usual exec/return wrapper but setsoverride_stmttohost_functions::override_factory_caller(). This ensures all factory-context no-mangle entrypoints (e.g.init,total_supply,pay_to_mint,approve) resolve the caller consistently via the factory override logic.
321-395:new_contractwasm entrypoint uses child entrypoints and events as expected
NoMangleFactoryFnItem’snew_contract:
- calls
override_factory_caller(),- builds child
entry_points()viachild_contract_entry_points(),- uses the child module’s
HasEventsimplementation for schemas, and- installs the new contract with optional init args assembled from the factory’s constructor, then returns
(Address, URef).This matches the intended factory behavior and the corresponding test expectation.
527-573: Tests accurately lock in the new factory/upgrade wasm layoutThe updated golden test in this file mirrors:
- the
entry_points()set including Template + Factory/FactoryUpgrade/FactoryBatchUpgrade variants,child_contract_entry_points()with constructor/regular entrypoints for the child,- the
new_contractwrapper with caller override and child install, and- the new upgrade and batch-upgrade entrypoints with the expected argument handling and event emission.
This provides good protection against regressions in the macro codegen for the factory paths.
Also applies to: 575-608, 628-667, 704-738
…nsistency across factory implementations
Benchmark report
|
…ts and simplify type handling
Benchmark report
|
Summary by CodeRabbit
New Features
Breaking Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.