Skip to content

Commit 9b64ea8

Browse files
kariyclaude
andauthored
fix(dojo-utils): return real address on already-deployed path (#3404)
* fix(dojo-utils): return real contract_address on already-deployed path `Deployer::deploy_via_udc` computes the deterministic UDC-derived contract address, checks `is_deployed` at that address, and when the contract is already there returns `Ok((Felt::ZERO, Noop))`. The real address is computed then dropped. Callers relying on the return value (including everything that expects idempotent deploys) see zero. The `deploy_via_udc_getcall` signature reinforces this — it returns `Option<(Felt, Call)>` where `None` means "already deployed", and the address computed along the way is lost. Change: - `deploy_via_udc_getcall` now returns `(Felt, Option<Call>)`. The address is always surfaced; the Option<Call> encodes whether a deploy call is needed. - `deploy_via_udc` unwraps that to `(address, TransactionResult::Noop)` on the already-deployed path, with the correct address. The one external caller in `sozo-ops::migrate` was pattern-matching `Some((_, call))` / `None` and discarding the address — it adapts to the new shape trivially (`let (_, call) = ...await?; deploy_call = call`), in fact becoming shorter and clearer. Breaking change for any direct consumer of `deploy_via_udc_getcall`, but the return type change is mechanical to fix at each site. Reproduction before this change: run `saya-ops core-contract deploy --salt X` twice against the same L2. First run succeeds, second run returns contract_address="0x0" in its JSON output, which propagates into any downstream orchestration and causes subsequent txns targeting the deployed contract to fail with "Requested contract address 0x0 is not deployed." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(dojo-utils): regression test for deploy_via_udc idempotency Spins up a katana_runner, deploys the predeclared account class at a fixed salt, then verifies both `deploy_via_udc_getcall` and `deploy_via_udc` return the real contract address on the already-deployed path. Before this change the tests would fail with address=Felt::ZERO on the second call; after, both paths return the deterministic UDC-derived address and deploy_via_udc returns TransactionResult::Noop. Uses the same harness pattern as the existing waiter tests (\`#[katana_runner::test]\` + \`RunnerCtx\`) so no new dev-deps needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(dojo-utils): rustfmt per nightly-2024-08-28 CI's fmt check uses the pinned nightly-2024-08-28 rustfmt which collapses the chained-await blocks and the multi-line assert_eq! in the new test onto single lines. Purely cosmetic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a6c6047 commit 9b64ea8

2 files changed

Lines changed: 91 additions & 22 deletions

File tree

crates/dojo/utils/src/tx/deployer.rs

Lines changed: 85 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,14 +35,19 @@ where
3535
Self { account, txn_config }
3636
}
3737

38-
/// Get a Call for deploying a contract via the UDC.
38+
/// Get the deterministic UDC-derived contract address along with the
39+
/// Call required to deploy it (or `None` for the Call if the contract
40+
/// is already deployed at that address).
41+
///
42+
/// The address is always returned, even on the already-deployed path,
43+
/// so callers don't have to re-derive it themselves.
3944
pub async fn deploy_via_udc_getcall(
4045
&self,
4146
class_hash: Felt,
4247
salt: Felt,
4348
constructor_calldata: &[Felt],
4449
deployer_address: Felt,
45-
) -> Result<Option<(Felt, Call)>, TransactionError<A::SignError>> {
50+
) -> Result<(Felt, Option<Call>), TransactionError<A::SignError>> {
4651
let udc_calldata = [
4752
vec![class_hash, salt, deployer_address, Felt::from(constructor_calldata.len())],
4853
constructor_calldata.to_vec(),
@@ -53,13 +58,13 @@ where
5358
get_contract_address(salt, class_hash, constructor_calldata, deployer_address);
5459

5560
if is_deployed(contract_address, &self.account.provider()).await? {
56-
return Ok(None);
61+
return Ok((contract_address, None));
5762
}
5863

59-
Ok(Some((
64+
Ok((
6065
contract_address,
61-
Call { calldata: udc_calldata, selector: UDC_DEPLOY_SELECTOR, to: UDC_ADDRESS },
62-
)))
66+
Some(Call { calldata: udc_calldata, selector: UDC_DEPLOY_SELECTOR, to: UDC_ADDRESS }),
67+
))
6368
}
6469

6570
/// Deploys a contract via the UDC.
@@ -70,12 +75,11 @@ where
7075
constructor_calldata: &[Felt],
7176
deployer_address: Felt,
7277
) -> Result<(Felt, TransactionResult), TransactionError<A::SignError>> {
73-
let (contract_address, call) = match self
78+
let (contract_address, call) = self
7479
.deploy_via_udc_getcall(class_hash, salt, constructor_calldata, deployer_address)
75-
.await?
76-
{
77-
Some(res) => res,
78-
None => return Ok((Felt::ZERO, TransactionResult::Noop)),
80+
.await?;
81+
let Some(call) = call else {
82+
return Ok((contract_address, TransactionResult::Noop));
7983
};
8084

8185
let InvokeTransactionResult { transaction_hash } =
@@ -121,3 +125,73 @@ where
121125
Err(e) => Err(e),
122126
}
123127
}
128+
129+
#[cfg(test)]
130+
mod tests {
131+
use katana_runner::RunnerCtx;
132+
use starknet::core::utils::get_contract_address;
133+
use starknet::macros::felt;
134+
135+
use super::*;
136+
use crate::TxnConfig;
137+
138+
// The default account class that katana dev predeclares on every chain.
139+
// Used as the class_hash for our deploy tests so we don't need to declare
140+
// a contract first.
141+
const KATANA_DEV_ACCOUNT_CLASS_HASH: Felt =
142+
felt!("0x07dc7899aa655b0aae51eadff6d801a58e97dd99cf4666ee59e704249e51adf2");
143+
144+
/// Regression: `deploy_via_udc_getcall` used to return `Option<(Felt, Call)>`
145+
/// where `None` meant "already deployed" and the address was dropped on
146+
/// the floor. `deploy_via_udc` then mapped that to `(Felt::ZERO, Noop)`.
147+
/// After the fix both paths surface the real contract address, so
148+
/// deploy is idempotent across re-runs with the same salt.
149+
#[tokio::test(flavor = "multi_thread")]
150+
#[katana_runner::test(accounts = 2)]
151+
async fn deploy_via_udc_idempotent_returns_real_address(sequencer: &RunnerCtx) {
152+
let account = sequencer.account(0);
153+
let deployer = Deployer::new(account, TxnConfig { wait: true, ..Default::default() });
154+
155+
let class_hash = KATANA_DEV_ACCOUNT_CLASS_HASH;
156+
let salt = felt!("0xabc");
157+
// Account class has a single-arg constructor (public_key). Any non-zero
158+
// felt works for this test; we never interact with the deployed account.
159+
let calldata = vec![felt!("0xdeadbeef")];
160+
let deployer_address = Felt::ZERO;
161+
162+
let expected_address = get_contract_address(salt, class_hash, &calldata, deployer_address);
163+
164+
// First call: not yet deployed. Returns (addr, Some(call)).
165+
let (addr, call) = deployer
166+
.deploy_via_udc_getcall(class_hash, salt, &calldata, deployer_address)
167+
.await
168+
.unwrap();
169+
assert_eq!(addr, expected_address);
170+
assert!(call.is_some(), "expected deploy Call on the not-yet-deployed path");
171+
172+
// Actually deploy it.
173+
let (deployed_addr, _tx) =
174+
deployer.deploy_via_udc(class_hash, salt, &calldata, deployer_address).await.unwrap();
175+
assert_eq!(deployed_addr, expected_address);
176+
177+
// Second getcall with identical params: contract is already deployed
178+
// at the same address. Returns (same addr, None) — this is the path
179+
// that used to lose the address before the fix.
180+
let (addr, call) = deployer
181+
.deploy_via_udc_getcall(class_hash, salt, &calldata, deployer_address)
182+
.await
183+
.unwrap();
184+
assert_eq!(addr, expected_address, "address must be surfaced even when already deployed");
185+
assert!(call.is_none(), "no deploy Call needed on the already-deployed path");
186+
187+
// Second deploy_via_udc call: returns (real_address, Noop). Before
188+
// the fix this returned (Felt::ZERO, Noop).
189+
let (addr, tx) =
190+
deployer.deploy_via_udc(class_hash, salt, &calldata, deployer_address).await.unwrap();
191+
assert_eq!(addr, expected_address);
192+
assert!(
193+
matches!(tx, TransactionResult::Noop),
194+
"already-deployed path must return Noop, got {tx:?}"
195+
);
196+
}
197+
}

crates/sozo/ops/src/migrate/mod.rs

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -917,23 +917,18 @@ where
917917

918918
let deployer = Deployer::new(&self.world.account, self.txn_config);
919919

920-
match deployer
920+
// `deploy_via_udc_getcall` returns the UDC-derived contract
921+
// address plus an Option<Call> — None means the contract is
922+
// already deployed at that address and no call is needed.
923+
let (_contract_address, call) = deployer
921924
.deploy_via_udc_getcall(
922925
contract.common.class_hash,
923926
contract.salt,
924927
&contract.encoded_constructor_data,
925928
Felt::ZERO,
926929
)
927-
.await?
928-
{
929-
Some((_, call)) => deploy_call = Some(call),
930-
None => {
931-
deploy_call = {
932-
// Already deployed, no need to deploy again.
933-
None
934-
}
935-
}
936-
}
930+
.await?;
931+
deploy_call = call;
937932

938933
is_upgradeable = contract.is_upgradeable;
939934
}

0 commit comments

Comments
 (0)