Skip to content

Add missing args in factory contract ref #626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kpob merged 3 commits into from
Nov 25, 2025
Merged

Add missing args in factory contract ref #626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad900da
Add missing args in factory contract ref
kpob Nov 25, 2025
515b92b
Ignore proxy test in odra vm environment
kpob Nov 25, 2025
5207dbb
Refactor factory contract functions to remove unnecessary caller over…
kpob Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/Odra.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,6 @@ fqn = "factory::token::FTokenFactory"

[[contracts]]
fqn = "factory::token::FToken"

[[contracts]]
fqn = "factory::token::FactoryProxy"
53 changes: 50 additions & 3 deletions examples/src/factory/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use odra::{casper_types::U256, prelude::*};
use odra::{casper_types::U256, prelude::*, ContractRef};
use odra_modules::{access::Ownable, cep18_token::Cep18};

#[odra::module(factory=on)]
Expand Down Expand Up @@ -32,17 +32,44 @@ impl FToken {
}
}

#[odra::module]
pub struct FactoryProxy {
factory_address: Var<Address>
}

#[odra::module]
impl FactoryProxy {
pub fn init(&mut self, address: Address) {
self.factory_address.set(address);
}

pub fn deploy_new_contract(&self) -> Address {
Copy link

@llamapreview llamapreview bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let factory_address = self.factory_address.get().unwrap_or_revert(self);

let mut factory = FTokenFactoryContractRef::new(self.env(), factory_address);
let (addr, _uref) = factory.new_contract(
"TokenContract".to_string(),
"Token".to_string(),
"TTK".to_string(),
18,
U256::from(1000u64)
);
addr
}
}

#[cfg(test)]
mod tests {
use alloc::string::ToString;
use odra::{
casper_types::U256,
host::{Deployer, HostRef, NoArgs}
host::{Deployer, HostRef, NoArgs},
prelude::Addressable
};

use crate::factory::token::{
FToken as Token, FTokenFactory as TokenFactory, FTokenHostRef,
FTokenInitArgs as TokenInitArgs
FTokenInitArgs as TokenInitArgs, FactoryProxy, FactoryProxyInitArgs
};

#[test]
Expand Down Expand Up @@ -84,4 +111,24 @@ mod tests {
assert_eq!(token.symbol(), "TTK".to_string());
assert_eq!(token.total_supply(), U256::from(500u64));
}

#[test]
Copy link

@llamapreview llamapreview bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[ignore = "This test does not work on odra vm"]
fn test_proxy() {
let env = odra_test::env();
let factory = TokenFactory::deploy(&env, NoArgs);
let proxy = FactoryProxy::deploy(
&env,
FactoryProxyInitArgs {
address: factory.address()
}
);

let addr = proxy.deploy_new_contract();
let token = FTokenHostRef::new(addr, env);
assert_eq!(token.get_owner(), proxy.address());
assert_eq!(token.name(), "Token".to_string());
assert_eq!(token.symbol(), "TTK".to_string());
assert_eq!(token.total_supply(), U256::from(1000u64));
}
}
5 changes: 0 additions & 5 deletions odra-casper/wasm-env/src/host_functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,6 @@ pub fn install_new_contract(

let contract_package_hash = ContractPackageHash::new(contract_hash.value());
if has_init {
if is_factory {
unsafe {
CALLER_OVERRIDE = true;
}
}
let init_access = create_contract_user_group(contract_package_hash, CONSTRUCTOR_GROUP_NAME);
let _: () = runtime::call_versioned_contract(
contract_package_hash,
Expand Down
25 changes: 10 additions & 15 deletions odra-macros/src/ast/factory/impl_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,15 @@ mod test {
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(true, "odra_cfg_is_upgradable", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(false, "odra_cfg_is_upgrade", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(contract_name.clone(), "odra_cfg_package_hash_key_name", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(contract_name, "contract_name", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(value, "value", &mut named_args);
named_args
}
)
.with_amount(self.attached_value),
)
}

Expand All @@ -145,14 +145,13 @@ mod test {
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, "contract_name", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_is_factory_upgrade", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(false, "odra_cfg_create_upgrade_group", &mut named_args);
named_args
}
)
.with_amount(self.attached_value),
)
}

Expand All @@ -166,14 +165,13 @@ mod test {
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(odra::args::BatchUpgradeArgs::from(args), "args", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_is_factory_upgrade", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(false, "odra_cfg_create_upgrade_group", &mut named_args);
named_args
}
)
.with_amount(self.attached_value),
)
}
}
Expand Down Expand Up @@ -632,7 +630,6 @@ mod test {

#[no_mangle]
fn total_supply() {
odra::odra_casper_wasm_env::host_functions::override_factory_caller();
let result = __erc20_factory_exec_parts::execute_total_supply(odra::odra_casper_wasm_env::WasmContractEnv::new_env());
odra::odra_casper_wasm_env::casper_contract::contract_api::runtime::ret(
odra::odra_casper_wasm_env::casper_contract::unwrap_or_revert::UnwrapOrRevert::unwrap_or_revert(
Expand All @@ -643,13 +640,11 @@ mod test {

#[no_mangle]
fn pay_to_mint() {
odra::odra_casper_wasm_env::host_functions::override_factory_caller();
__erc20_factory_exec_parts::execute_pay_to_mint(odra::odra_casper_wasm_env::WasmContractEnv::new_env());
}

#[no_mangle]
fn approve() {
odra::odra_casper_wasm_env::host_functions::override_factory_caller();
__erc20_factory_exec_parts::execute_approve(odra::odra_casper_wasm_env::WasmContractEnv::new_env());
}
}
Expand Down
36 changes: 13 additions & 23 deletions odra-macros/src/ast/factory/parts/ref_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl TryFrom<&'_ ModuleImplIR> for ContractRefImplItem {
let fns = vec![module.factory_fn(), module.factory_upgrade_fn(), module.factory_batch_upgrade_fn()];
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()
})
Comment on lines 32 to 35
Copy link

@llamapreview llamapreview bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Expand Down Expand Up @@ -96,15 +96,15 @@ mod test {
true,
Copy link

@llamapreview llamapreview bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

{
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(true, "odra_cfg_is_upgradable", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(false, "odra_cfg_is_upgrade", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(contract_name.clone(), "odra_cfg_package_hash_key_name", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(contract_name, "contract_name", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(value, "value", &mut named_args);
named_args
}
)
.with_amount(self.attached_value),
)
}

Expand All @@ -120,18 +120,13 @@ mod test {
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,
"contract_name",
&mut named_args,
);
odra::args::EntrypointArgument::insert_runtime_arg(contract_name, "contract_name", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_is_factory_upgrade", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(false, "odra_cfg_create_upgrade_group", &mut named_args);
named_args
},
)
.with_amount(self.attached_value),
)
}

Expand All @@ -151,18 +146,13 @@ mod test {
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(
odra::args::BatchUpgradeArgs::from(args),
"args",
&mut named_args,
);
odra::args::EntrypointArgument::insert_runtime_arg(odra::args::BatchUpgradeArgs::from(args), "args", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_is_factory_upgrade", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(true, "odra_cfg_allow_key_override", &mut named_args);
odra::args::EntrypointArgument::insert_runtime_arg(false, "odra_cfg_create_upgrade_group", &mut named_args);
named_args
},
)
.with_amount(self.attached_value),
)
}
}
Expand Down
12 changes: 8 additions & 4 deletions odra-macros/src/ast/factory/parts/wasm_parts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use syn::parse_quote;
use crate::ast::parts_utils::{UsePreludeItem, UseSuperItem};
use crate::ast::wasm_parts::{NoMangleFnItem, NoMangleItemContext};
use crate::ast::wasm_parts_utils;
use crate::ir::FnType;
use crate::utils::misc::AsType;
use crate::{
ast::fn_utils,
Expand Down Expand Up @@ -35,9 +36,15 @@ impl TryFrom<(&'_ ModuleImplIR, &'_ FnIR)> for NoMangleFnItem<FactoryContext> {
syn::ReturnType::Type(_, _) => Some(utils::stmt::runtime_return(&result_ident))
};

let override_stmt = if [FnType::Constructor, FnType::Upgrader].contains(&func.fn_type()) {
Some(parse_quote!(odra::odra_casper_wasm_env::host_functions::override_factory_caller();))
} else {
None
};

Ok(Self {
sig: parse_quote!(fn #fn_ident()),
override_stmt: Some(parse_quote!(odra::odra_casper_wasm_env::host_functions::override_factory_caller();)),
override_stmt,
execute_stmt,
ret_stmt,
ctx: std::marker::PhantomData::<FactoryContext>
Expand Down Expand Up @@ -745,7 +752,6 @@ mod test {

#[no_mangle]
fn total_supply() {
odra::odra_casper_wasm_env::host_functions::override_factory_caller();
let result = __erc20_factory_exec_parts::execute_total_supply(odra::odra_casper_wasm_env::WasmContractEnv::new_env());
odra::odra_casper_wasm_env::casper_contract::contract_api::runtime::ret(
odra::odra_casper_wasm_env::casper_contract::unwrap_or_revert::UnwrapOrRevert::unwrap_or_revert(
Expand All @@ -756,13 +762,11 @@ mod test {

#[no_mangle]
fn pay_to_mint() {
odra::odra_casper_wasm_env::host_functions::override_factory_caller();
__erc20_factory_exec_parts::execute_pay_to_mint(odra::odra_casper_wasm_env::WasmContractEnv::new_env());
}

#[no_mangle]
fn approve() {
odra::odra_casper_wasm_env::host_functions::override_factory_caller();
__erc20_factory_exec_parts::execute_approve(odra::odra_casper_wasm_env::WasmContractEnv::new_env());
}
}
Expand Down
12 changes: 12 additions & 0 deletions odra-macros/src/ast/ref_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ pub fn contract_function_item(fun: &FnIR, is_trait_impl: bool) -> syn::ItemFn {
env_call(signature, call_def_expr, attrs, vis)
}

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)
Comment on lines +60 to +69
Copy link

@llamapreview llamapreview bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

fn env_call(
sig: syn::Signature,
call_def_expr: syn::Expr,
Expand Down