Add missing args in factory contract ref #626
Conversation
![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 critical breaking change by removing attached value handling in factory contract references, which could silently fail existing deployments that rely on token transfers, alongside maintainability and testing concerns.
📄 Documentation Diagram
This diagram documents the new FactoryProxy workflow for deploying contracts via factory references.
sequenceDiagram
participant U as User
participant P as FactoryProxy
participant F as FTokenFactory
participant C as FToken
U->>P: deploy_new_contract()
P->>F: new_contract("TokenContract", "Token", "TTK", 18, 1000)
note over F: PR #35;626 changed call without attached value
F->>C: deploy
C-->>F: address
F-->>P: address
P-->>U: address
🌟 Strengths
- Improves code organization by separating factory contract reference generation.
- Adds a new FactoryProxy example for better demonstration of factory usage.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | odra-macros/.../ref_item.rs | Architecture | Removes attached value, breaking token transfers | path:odra-vm/src/vm/odra_vm.rs |
| P2 | odra-macros/.../ref_item.rs | Maintainability | Adds new function, risk of divergence | |
| P2 | examples/.../token.rs | Testing | Ignores test, may mask issues | |
| P2 | examples/.../token.rs | Architecture | Proxy lacks error handling for failures | |
| P2 | odra-macros/.../ref_utils.rs | Maintainability | Code duplication in function generation |
🔍 Notable Themes
- Breaking Changes: Removal of attached value in factory methods introduces silent failures for existing token-based deployments.
- Code Organization: New separation of factory reference generation improves clarity but requires careful maintenance to avoid divergence.
- Testing Gaps: Ignored tests and lack of error handling in examples could lead to undetected issues in production.
📈 Risk Diagram
This diagram illustrates the risk of removed attached value in factory contract deployments.
sequenceDiagram
participant Caller
participant FCR as FactoryContractRef
participant VM as Odra VM
Caller->>FCR: new_contract(...)
FCR->>VM: call_contract without amount
note over VM: R1(P1): Risk of failed token transfers if amount was needed
VM-->>FCR: result
FCR-->>Caller: result
💡 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.
| Ok(Self { | ||
| ref_ident: module.contract_ref_ident()?, | ||
| factory_fns: fns.iter().map(|fun| ref_utils::contract_function_item(fun, false)).collect() | ||
| factory_fns: fns.iter().map(|fun| ref_utils::factory_contract_function_item(fun, false)).collect() | ||
| }) |
There was a problem hiding this comment.
P2 | Confidence: High
The change introduces a new function factory_contract_function_item for generating factory contract reference methods, separating it from regular contract references. This improves code organization but creates a potential maintenance risk if the two code generation paths diverge unnecessarily. The separation should be clearly documented to prevent future inconsistencies.
Code Suggestion:
Add comments explaining the distinction between regular and factory contract reference generation.| assert_eq!(token.total_supply(), U256::from(500u64)); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
P2 | Confidence: High
A new test has been added but immediately ignored with a VM-specific reason. While this documents known limitations, ignored tests can become stale and mask real issues. The test should either be fixed to work in all environments or have a clear plan for when it will be enabled.
Code Suggestion:
Consider adding a conditional compilation attribute or environment-specific test configuration instead of blanket ignoring.| self.factory_address.set(address); | ||
| } | ||
|
|
||
| pub fn deploy_new_contract(&self) -> Address { |
There was a problem hiding this comment.
P2 | Confidence: Medium
The FactoryProxy example demonstrates the new factory contract reference usage but doesn't handle potential errors from the factory call. In a production scenario, factory deployments might fail due to various reasons (insufficient balance, invalid parameters), and the proxy should handle these gracefully rather than relying on the environment to revert.
Code Suggestion:
Consider adding error handling or at least documenting the expected failure modes.| pub fn factory_contract_function_item(fun: &FnIR, is_trait_impl: bool) -> syn::ItemFn { | ||
| let vis = match is_trait_impl { | ||
| true => visibility_default(), | ||
| false => visibility_pub() | ||
| }; | ||
| let signature = function_signature(fun); | ||
| let call_def_expr = factory_call_def_with_amount(fun); | ||
| let attrs = function_filtered_attrs(fun); | ||
|
|
||
| env_call(signature, call_def_expr, attrs, vis) |
There was a problem hiding this comment.
P2 | Confidence: Medium
The new factory_contract_function_item function closely mirrors the existing contract_function_item but uses factory_call_def_with_amount. This duplication could lead to maintenance overhead if both functions need similar future modifications. Consider whether these could be unified with a configuration parameter.
| self.address, | ||
| odra::CallDef::new( | ||
| odra::prelude::string::String::from("new_contract"), | ||
| true, |
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 91. Anchored to nearest_changed(96) line 96.
P1 | Confidence: High
The factory contract reference methods (new_contract, upgrade_child_contract, batch_upgrade_child_contract) have been modified to remove the attached value (amount) from the call definitions. This is a breaking change for any existing code that relies on sending tokens with factory deployments. The related context shows that the VM's call_contract method does handle the attached value by transferring tokens (see odra-vm/src/vm/odra_vm.rs lines 114-118). Any existing factory deployments that require token transfers will now fail silently or behave incorrectly.
Code Suggestion:
Either preserve the attached value mechanism or provide a clear migration path and documentation for alternative token transfer methods.Evidence: path:odra-vm/src/vm/odra_vm.rs
…ride and streamline argument handling
Benchmark report
|
No description provided.