fix: demo deploy verification and README corrections#296
Conversation
0xrcinus
requested review from
NicoMolinaOZ,
nahimterrazas,
pepebndc and
shahnami
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors solver-demo deployment to verify per-chain contracts on-chain (skip or redeploy) and run per-chain deployment tasks in parallel; adjusts Permit2 bytecode path resolution; updates docs/Quick Start to require Rust 1.88.0+, add oif-contracts clone/build steps, and standardize token CLI flag syntax; fixes a README JSON snippet. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant EnvOps as EnvOps
participant Session as Session
participant Provider as Provider
participant Deployer as Deployer
CLI->>EnvOps: start deploy (chains, force?)
EnvOps->>Session: get local contracts for chain
alt no local contracts
EnvOps->>Deployer: deploy chain
else force == true
EnvOps->>Deployer: deploy chain
else local contracts exist and not forced
EnvOps->>Provider: fetch code at configured address
Provider-->>EnvOps: code or error
alt code non-empty
EnvOps->>EnvOps: log "Skipping chain (contracts verified on-chain)"
else code empty
EnvOps->>Deployer: deploy chain (re-deploy)
else error
EnvOps->>Deployer: deploy chain (could not verify on-chain)
end
end
Deployer-->>EnvOps: deployed addresses
EnvOps->>Session: store deployment addresses
EnvOps->>CLI: report completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 986-987: Update the README entry that currently reads "Rust
toolchain (stable, 1.88.0+)" to remove the plus so it matches the pinned version
in rust-toolchain.toml; change the line to "Rust toolchain (stable, 1.88.0)" to
reflect the exact locked toolchain.
🧹 Nitpick comments (1)
crates/solver-demo/src/operations/env/mod.rs (1)
289-317: Checking onlyinput_settlermay miss partial deployment failures.If a previous deployment crashed mid-way,
input_settlercould have code on-chain while later contracts (e.g.,output_settler,allocator) were never deployed. The demo would then skip redeployment, leaving the environment incomplete.For a demo tool this is likely acceptable, but consider documenting that
--force(orenv deploy --all) should be used if a deployment was interrupted, or check one more contract (e.g., the last one deployed:output_oracle) as a basic sanity check.
0xrcinus
force-pushed
the
fix/demo-deploy-and-readme-corrections
branch
from
c633080 to
3f943e3
Compare
nahimterrazas
left a comment
nahimterrazas
left a comment
There was a problem hiding this comment.
Thanks @0xrcinus! LGTM.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
same here! + clippy @0xrcinus thanks! |
0xrcinus
force-pushed
the
fix/demo-deploy-and-readme-corrections
branch
from
3f943e3 to
ab9eb98
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/solver-demo/src/operations/env/mod.rs (1)
190-196: Log the verification error for debuggability.The
Err(_)wildcard discards the actual error, making it harder to diagnose why on-chain verification failed (e.g., RPC timeout, DNS failure, invalid config). Include it in the log message.🔧 Proposed fix
- Err(_) => { + Err(e) => { logging::verbose_operation( "Re-deploying chain", - &format!("{chain} (could not verify on-chain)"), + &format!("{chain} (could not verify on-chain: {e})"), ); true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-demo/src/operations/env/mod.rs` around lines 190 - 196, The Err(_) arm discards the verification error; change the match to capture the error (e.g., Err(e)) and include its details in the logging::verbose_operation message so the log shows the chain and the underlying error (reference the Err(_) pattern in the match and the logging::verbose_operation call in mod.rs) — ensure the message concatenates or formats the error (e) so RPC/DNS/config errors are visible for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/solver-demo/src/operations/env/mod.rs`:
- Around line 190-196: The Err(_) arm discards the verification error; change
the match to capture the error (e.g., Err(e)) and include its details in the
logging::verbose_operation message so the log shows the chain and the underlying
error (reference the Err(_) pattern in the match and the
logging::verbose_operation call in mod.rs) — ensure the message concatenates or
formats the error (e) so RPC/DNS/config errors are visible for debugging.
|
Thanks @nahimterrazas , should be fixed now! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/solver-demo/src/operations/env/mod.rs (2)
188-194: Include the error in the "could not verify on-chain" log message.The
Err(_)arm drops the error value entirely. When verification fails for any reason (provider unreachable, misconfigured RPC URL, network timeout, etc.) the log only says "could not verify on-chain", giving no diagnostic information. The resulting re-deploy might also be undesirable (e.g., contracts are deployed but the RPC endpoint is temporarily down).♻️ Proposed fix
- Err(_) => { + Err(e) => { logging::verbose_operation( "Re-deploying chain", - &format!("{chain} (could not verify on-chain)"), + &format!("{chain} (could not verify on-chain: {e})"), ); true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-demo/src/operations/env/mod.rs` around lines 188 - 194, Change the Err(_) arm so it captures the error (e.g., Err(e)) instead of discarding it, and include that error in the verbose log for the "Re-deploying chain" message; update the call to logging::verbose_operation that currently formats "{chain} (could not verify on-chain)" to include the error (e.g., "{chain} (could not verify on-chain: {error})") so callers of the branch can see provider/RPC/timeouts or other diagnostics—the change should be made where the match/closure contains the Err(_) arm that returns true and uses logging::verbose_operation.
297-314:verify_contracts_onchainchecks onlyinput_settleras a proxy for all contracts.If
input_settlerwas deployed in a prior run but other contracts (e.g.,output_settler,permit2, tokens) were not — a partial-deploy scenario — this function returnsOk(true)and the entire chain is skipped, leaving the environment in a broken state. For an atomically-deployed demo this is low risk, but worth noting as the single check can mask partial deployment failures.Additionally, if
contracts.input_settlerisNonewhilesession.has_contracts(chain)istrue(a plausible config state), this returnsErr(InvalidConfig(...))which is silently swallowed by theErr(_)arm and triggers a re-deploy. The user sees "could not verify on-chain" with no indication that the root cause is a missinginput_settlerfield in the config.Consider checking a second independent contract (e.g.,
output_settlerorpermit2) to reduce the probability of a false positive, or document the single-check limitation explicitly in the function doc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-demo/src/operations/env/mod.rs` around lines 297 - 314, The verify_contracts_onchain function currently only probes contracts.input_settler which can give false positives on partial deployments and hides missing input_settler when session.has_contracts(chain) is true; update verify_contracts_onchain to validate at least two independent contracts (e.g., contracts.input_settler and contracts.output_settler, or input_settler and contracts.permit2) by parsing both addresses (Address::parse) and fetching both codes via provider.inner().get_code_at(addr).await, and only return Ok(true) if both codes are present; also change the error handling so a missing input_settler (None) when session.has_contracts(chain) is true returns or propagates Error::InvalidConfig with a clear message instead of being silently mapped to a generic "could not verify on-chain" path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/solver-demo/src/operations/env/mod.rs`:
- Around line 188-194: Change the Err(_) arm so it captures the error (e.g.,
Err(e)) instead of discarding it, and include that error in the verbose log for
the "Re-deploying chain" message; update the call to logging::verbose_operation
that currently formats "{chain} (could not verify on-chain)" to include the
error (e.g., "{chain} (could not verify on-chain: {error})") so callers of the
branch can see provider/RPC/timeouts or other diagnostics—the change should be
made where the match/closure contains the Err(_) arm that returns true and uses
logging::verbose_operation.
- Around line 297-314: The verify_contracts_onchain function currently only
probes contracts.input_settler which can give false positives on partial
deployments and hides missing input_settler when session.has_contracts(chain) is
true; update verify_contracts_onchain to validate at least two independent
contracts (e.g., contracts.input_settler and contracts.output_settler, or
input_settler and contracts.permit2) by parsing both addresses (Address::parse)
and fetching both codes via provider.inner().get_code_at(addr).await, and only
return Ok(true) if both codes are present; also change the error handling so a
missing input_settler (None) when session.has_contracts(chain) is true returns
or propagates Error::InvalidConfig with a clear message instead of being
silently mapped to a generic "could not verify on-chain" path.
|
Thanks @0xrcinus, the project required that commits must have verified signatures, so I can merge, apparently one of your comments was not signed. Also, verify Clippy too |
0xrcinus
force-pushed
the
fix/demo-deploy-and-readme-corrections
branch
from
97b6bd6 to
a0a9d6d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/solver-demo/src/operations/env/mod.rs (1)
188-194: Log the verification error instead of discarding it.The
Err(_)arm silently drops the underlying cause (RPC failure, parse error, etc.). When a user hits this branch unexpectedly, the log line gives no actionable signal.♻️ Proposed fix
- Err(_) => { + Err(e) => { logging::verbose_operation( "Re-deploying chain", - &format!("{chain} (could not verify on-chain)"), + &format!("{chain} (could not verify on-chain: {e})"), ); true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-demo/src/operations/env/mod.rs` around lines 188 - 194, The Err(_) match arm that calls logging::verbose_operation when on-chain verification fails should capture the error (e.g., Err(err)) and include its details in the log instead of discarding it; change the pattern to Err(err) and update the formatted message passed to logging::verbose_operation (or add a second log call) to include err (use Debug/Display as appropriate) so RPC/parsing errors are visible during "Re-deploying chain" handling.README.md (1)
1056-1058:git cloneis not idempotent — re-running Quick Start will error ifoif-contracts/already exists.A user who re-runs these steps (e.g., after pulling updates or rebuilding) will get
fatal: destination path 'oif-contracts' already exists.📝 Proposed fix for idempotent clone
-# 0. Clone and build the OIF contracts (needed for local deployment) -git clone https://github.com/openintentsframework/oif-contracts.git -cd oif-contracts && forge build && cd .. +# 0. Clone and build the OIF contracts (needed for local deployment) +[ -d oif-contracts ] || git clone https://github.com/openintentsframework/oif-contracts.git +cd oif-contracts && forge build && cd ..🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 1056 - 1058, The README's clone step isn't idempotent: replace the single "git clone https://github.com/openintentsframework/oif-contracts.git" line with a safe check that skips cloning if the oif-contracts directory exists (or updates it), e.g. use a shell guard like: if [ -d oif-contracts ]; then cd oif-contracts && git pull --rebase; else git clone https://github.com/openintentsframework/oif-contracts.git; fi && cd oif-contracts && forge build && cd .. so re-running Quick Start won't fail when oif-contracts already exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Line 1048: Update the README entry that currently reads "Rust toolchain
(stable, 1.88.0+)" to remove the trailing plus so it matches the exact pinned
version in rust-toolchain.toml; replace the string with "Rust toolchain (stable,
1.88.0)" to ensure consistency with the pinned 1.88.0 toolchain.
---
Nitpick comments:
In `@crates/solver-demo/src/operations/env/mod.rs`:
- Around line 188-194: The Err(_) match arm that calls
logging::verbose_operation when on-chain verification fails should capture the
error (e.g., Err(err)) and include its details in the log instead of discarding
it; change the pattern to Err(err) and update the formatted message passed to
logging::verbose_operation (or add a second log call) to include err (use
Debug/Display as appropriate) so RPC/parsing errors are visible during
"Re-deploying chain" handling.
In `@README.md`:
- Around line 1056-1058: The README's clone step isn't idempotent: replace the
single "git clone https://github.com/openintentsframework/oif-contracts.git"
line with a safe check that skips cloning if the oif-contracts directory exists
(or updates it), e.g. use a shell guard like: if [ -d oif-contracts ]; then cd
oif-contracts && git pull --rebase; else git clone
https://github.com/openintentsframework/oif-contracts.git; fi && cd
oif-contracts && forge build && cd .. so re-running Quick Start won't fail when
oif-contracts already exists.
nahimterrazas
merged commit 9679337
into
openintentsframework:main
3 participants
Summary
This PR fixes several issues encountered when running the solver-demo locally, along with README documentation corrections.
Code Fixes
On-chain contract verification before skipping deployment (
crates/solver-demo/src/operations/env/mod.rs)When
init loadis used with a config file that already contains contract addresses (e.g., the checked-inconfig/demo.toml), the session gets populated with those addresses. Previously,env deploywould see these addresses and silently skip deployment — even on a fresh Anvil where no contracts exist. Theenv setupstep would then mint tokens to nonexistent contracts, leaving all balances at 0 with no error.The
deploy()method now callsverify_contracts_onchain()which does aneth_getCodecheck on the input_settler address. If no code is found, deployment proceeds.Fix Permit2 bytecode path (
crates/solver-demo/src/operations/env/deploy.rs)Changed from
CARGO_MANIFEST_DIR/src/operations/env/data/permit2_bytecode.hextoCARGO_MANIFEST_DIR/data/permit2_bytecode.hex, matching the documented location incrates/solver-demo/data/.README Fixes
token balance,token mint, andtoken approveexamples to use the correct flag syntax (--account,--chain,--token,--amount,--spender) instead of positional args"user": "0x74...,→"user": "0x74...",)rust-toolchain.tomlforge buildstep to the solver-demo Quick Start since the demo requires compiled contract artifacts inoif-contracts/out/Testing
init load→env start→env deploy→env setup→token balanceshows correct 1000 token balancesinit load config/demo.tomlon fresh Anvil) now works correctlySummary by CodeRabbit
Documentation
Improvements
Bug Fixes