feat: bonsai to boundless migration [skip-line-limit]#1030
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds a program upload command and Pinata upload scripts, migrates compute from Risc0/Bonsai to Boundless (new provider and dependencies), converts webhook payloads to status-tagged enums (Completed/Failed) with error tracking, extends configs to include Boundless settings, and adds two ciphernodes (CN4, CN5). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SupportApp
participant Boundless
participant WebhookReceiver
Client->>SupportApp: POST /compute (FHEInputs)
SupportApp->>SupportApp: process_computation_background()
rect rgb(220,235,255)
SupportApp->>Boundless: submit proof request (boundless-market)
Boundless-->>SupportApp: fulfillment (seal + ciphertext) / error
end
alt Success
SupportApp->>SupportApp: extract journal & result
SupportApp->>WebhookReceiver: POST {status:"completed", e3_id, ciphertext, proof}
WebhookReceiver-->>SupportApp: 200 OK
else Failure
SupportApp->>WebhookReceiver: POST {status:"failed", e3_id, error}
WebhookReceiver-->>SupportApp: 200 OK
end
sequenceDiagram
participant CLI
participant SupportScripts
participant ContainerScript
participant Pinata
CLI->>SupportScripts: program_upload(config)
SupportScripts->>SupportScripts: dispatch ProgramSupport variant
alt Dev
SupportScripts->>ContainerScript: run local upload (ctl/upload)
else Boundless/Risc0
SupportScripts->>ContainerScript: run ctl/upload with --pinata-jwt
ContainerScript->>Pinata: pinFileToIPFS (curl)
Pinata-->>ContainerScript: IPFS CID / error
ContainerScript->>ContainerScript: store hash & url (idempotent)
end
SupportScripts-->>CLI: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
hmzakhalid
changed the title
hmzakhalid
force-pushed
the
hmza/boundless-migration
branch
from
46e6c4d to
7bb0529
Compare
hmzakhalid
force-pushed
the
hmza/boundless-migration
branch
from
7bb0529 to
e562626
Compare
hmzakhalid
force-pushed
the
hmza/boundless-migration
branch
from
248c7a2 to
ef6bb1f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
examples/CRISP/server/src/server/models.rs (1)
15-15: Consider using an enum instead of String for type safety.The
statusfield only accepts two values ("completed" or "failed"), but using aStringallows any value and requires runtime validation. An enum would provide compile-time guarantees and prevent typos.Apply this diff:
+#[derive(Debug, Deserialize, Serialize)] +#[serde(rename_all = "lowercase")] +pub enum ComputationStatus { + Completed, + Failed, +} + #[derive(Derivative, Deserialize)] #[derivative(Debug)] pub struct WebhookPayload { pub e3_id: u64, - pub status: String, // "completed" or "failed" + pub status: ComputationStatus, #[serde(deserialize_with = "deserialize_hex_string_optional", default)]crates/config/src/app_config.rs (2)
93-112: Avoid derivingDebug/PartialEqon structs holding private keys and JWTs
BoundlessConfigcontains highly sensitive data (private_key,pinata_jwt), but the struct derivesDebugandPartialEq. Any accidental{:?}log or panic that includes this struct will leak these secrets, and equality-based debugging/tests may also surface them.Consider:
- Removing
Debug/PartialEqfromBoundlessConfig, or- Wrapping secret fields in a type with a redacted
Debugimplementation.This keeps the new Boundless config in line with secure handling of signing keys and tokens.
487-555: Add a dedicated test for Boundless config deserialization/defaultsThe updated
test_deserializationnow coversboundless: Nonewhen onlyrisc0_dev_modeis set, which is good. There’s currently no test that exercises:
- Required fields inside
BoundlessConfig(rpc_url,private_key) being present/missing.- Defaults for
pinata_jwt,program_url, andonchain(viadefault_true).A small YAML fixture with a
program.risc0.boundlesssection would lock in the expected behavior and catch regressions in serde attributes.crates/support/host/src/lib.rs (3)
128-130: Avoidunwrap()on serialization when buildinginput_bytesHere:
let input_bytes = encode_input(&serialize(input).unwrap()) .context("Failed to encode input")?;
serialize(input).unwrap()will panic on any serialization failure, bypassing your niceanyhow::Contextchain.Safer pattern:
- let input_bytes = encode_input(&serialize(input).unwrap()) - .context("Failed to encode input")?; + let serialized = + serialize(input).context("Failed to serialize compute input with bincode")?; + let input_bytes = + encode_input(&serialized).context("Failed to encode input for RISC0 guest")?;That way all failure modes are reported via
BoundlessOutput::Errorinstead of panicking.
28-38: Consider removing duplicate journal bytes inBoundlessOutputvsrun_computereturn
BoundlessOutput::Successcontains abytes: Vec<u8>, andrun_computealso returns aVec<u8>alongside theBoundlessOutput. This means the journal bytes are stored twice on the success path.If callers only need one copy, you could simplify to either:
- Keep bytes only inside
BoundlessOutput::Successand changerun_computeto returnBoundlessOutputalone, or- Drop the
bytesfield fromBoundlessOutput::Successand use only the tuple element.Not urgent, but trimming the duplication will reduce confusion and a bit of allocation/churn.
Also applies to: 218-252
40-44:ComputeError::Otheris currently unused
ComputeErrordefines anOther(String)variant that isn’t constructed anywhere in this file. If there’s no near-term use planned, consider removing it to keep the public API tight or start using it to distinguish non-Boundless failures.Not a blocker, just a small cleanup opportunity.
crates/support-scripts/ctl/upload (1)
1-24: JWT forwarding into container upload path looks correctThe script cleanly:
- Parses
--pinata-jwtand stores it once.- Constructs
CONTAINER_ARGS=("./scripts/container/upload.sh")and appends--pinata-jwtonly when provided.- Delegates to the existing
containerwrapper viaexec "$SCRIPT_DIR/container" "${CONTAINER_ARGS[@]}".This matches the usual ctl/container pattern. If you later need additional flags, you may want to collect unknown args into an array and pass them through instead of hard-failing, but for a single-purpose upload entrypoint this is fine.
crates/cli/src/program.rs (1)
31-33: CLIprogram uploadwiring to support-scripts looks goodThe new
Uploadsubcommand and its handler:ProgramCommands::Upload => { e3_support_scripts::program_upload(config.program().clone(), None).await? }hook the CLI cleanly into the
program_uploadpath using the existingProgramConfig. This is consistent withStart/Compilewiring.If you later want dev/prod-specific upload behavior, you could add a
dev: Option<bool>flag toUploadand thread it through instead of always passingNone, but the current behavior is coherent.Also applies to: 56-58
crates/support-scripts/ctl/start (1)
3-36: Argument parsing and boundless-related flags look consistent and safe
RPC_URL/PRIVATE_KEYmutual requirement is implemented correctly and will catch partial configs early.- Quoting of values when building
CONTAINER_ARGSavoids whitespace/shell injection issues.- Optional: if you ever want
--boundless-onchainto be a pure boolean flag, you could default its value (e.g.,1) when the flag is present without a second argument; current “expects a value” behavior is also fine and explicit.Also applies to: 41-63, 65-65
crates/support/scripts/container/upload.sh (1)
1-116: Upload workflow is solid; consider small robustness/UX tweaks
- The hash/idempotence logic, metadata printing, and reuse of
.program_hash/.program_urlare well thought out and should avoid unnecessary uploads.- Because
set -eis enabled andcurlis invoked with-s, network/DNS failures will cause the script to exit quietly before you hit the"Upload failed:"branch. Dropping-sor wrapping the curl call to print a clear error on non‑zero exit would improve debuggability.- Parsing
IpfsHashwithgrep | cutworks against the current Pinata response shape but is brittle; if you’re comfortable depending onjq, switching tojq -r '.IpfsHash'would make this more future‑proof.crates/program-server/src/lib.rs (2)
14-15: Webhook payload/status wiring is correct; HTTP error handling can be slightly simplified
- Adding
status: ComputationStatusanderror: Option<String>into the payload is wired correctly, and the extra logging of status plus ciphertext/proof lengths is useful.- The pattern of:
- manual
if !response.status().is_success()that reads and returns the body, and thenresponse.error_for_status()?;
is a bit redundant, since the second call can no longer fail for non‑2xx statuses. You can either:- keep the manual branch (for richer error messages) and drop
error_for_status(), or- rely solely on
error_for_status()and, if needed, read the body inside amatchon the error.
Not a blocker, just a small cleanup opportunity.Also applies to: 138-181
183-235: Background compute + status-aware webhook flow looks good; note error precedence
- The success branch correctly marks status as
Completed, forwards actual proof/ciphertext, and logs completion.- The error branch logs the compute failure and sends a
Failedwebhook with empty payloads and a human-readable error string, which is a sensible contract for consumers.- Be aware that if the webhook call itself fails in the error branch, the returned error will be from the webhook call (due to
await?), not the original compute error. Since the compute error is already logged, this is acceptable; if you ever want to preserve both, you could wrap them into a single combined error.crates/support/app/src/main.rs (2)
9-11: call_webhook implementation is sound; consider deduplicating with program-server
- Payload construction and status/error propagation match the shared
WebhookPayloadshape, and the extra pretty‑printed JSON log is useful for debugging.- The implementation is almost identical to
call_webhookincrates/program-server/src/lib.rs; if you expect to evolve this behavior (e.g., headers, retries, logging) it might be worth factoring into a shared helper to avoid drift.Also applies to: 18-61
63-130: Computation runner mapping and status-aware background processing are coherent
run_computation_asynccorrectly offloadse3_support_host::run_computeto a blocking thread and maps bothBoundlessOutputandComputeErrorvariants into meaningful error messages.- Given the current
run_computeimplementation already turnsBoundlessOutput::ErrorintoComputeError::BoundlessFailed, the innerBoundlessOutput::Errorarm in theOkbranch is probably unreachable, but leaving it in as a defensive check is harmless.process_computation_backgroundmirrors the program-server’s behavior:Completedstatus with real proof/ciphertext on success,Failedstatus with empty payloads and an error message on failure. This is consistent and easy for webhook consumers to handle.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/support/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
crates/cli/src/program.rs(2 hunks)crates/config/src/app_config.rs(2 hunks)crates/program-server/src/lib.rs(3 hunks)crates/program-server/src/types.rs(3 hunks)crates/support-scripts/ctl/start(2 hunks)crates/support-scripts/ctl/upload(1 hunks)crates/support-scripts/src/lib.rs(1 hunks)crates/support-scripts/src/program.rs(1 hunks)crates/support-scripts/src/program_dev.rs(1 hunks)crates/support-scripts/src/program_risc0.rs(1 hunks)crates/support-scripts/src/traits.rs(1 hunks)crates/support/Cargo.toml(2 hunks)crates/support/app/src/main.rs(2 hunks)crates/support/host/Cargo.toml(2 hunks)crates/support/host/src/lib.rs(2 hunks)crates/support/scripts/container/start.sh(2 hunks)crates/support/scripts/container/upload.sh(1 hunks)crates/support/types/src/lib.rs(1 hunks)deploy/local/contracts.sh(2 hunks)deploy/local/nodes.sh(1 hunks)examples/CRISP/enclave.config.yaml(1 hunks)examples/CRISP/packages/crisp-contracts/lib/risc0-ethereum(1 hunks)examples/CRISP/server/src/server/models.rs(2 hunks)examples/CRISP/server/src/server/routes/state.rs(2 hunks)templates/default/enclave.config.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2024-09-26T04:12:09.345Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:103-114
Timestamp: 2024-09-26T04:12:09.345Z
Learning: In `tests/basic_integration/test.sh`, the user prefers not to refactor the ciphernode addition section to reduce duplication.
Applied to files:
deploy/local/nodes.shdeploy/local/contracts.sh
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/support/Cargo.tomldeploy/local/contracts.shcrates/support/host/Cargo.toml
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/support/Cargo.tomlcrates/support/host/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
crates/support/Cargo.toml
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
examples/CRISP/server/src/server/routes/state.rs
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
deploy/local/contracts.sh
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
deploy/local/contracts.sh
📚 Learning: 2024-10-28T12:00:09.010Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave/src/commands/wallet/set.rs:17-23
Timestamp: 2024-10-28T12:00:09.010Z
Learning: In the `enclave` package of the `ciphernode` project, prefer using `println!` over logging macros like `error!` from the `tracing` crate for error output in CLI commands.
Applied to files:
deploy/local/contracts.sh
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
deploy/local/contracts.shexamples/CRISP/packages/crisp-contracts/lib/risc0-ethereumexamples/CRISP/enclave.config.yaml
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
deploy/local/contracts.sh
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
deploy/local/contracts.sh
📚 Learning: 2024-10-23T01:59:27.215Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: tests/basic_integration/test.sh:21-21
Timestamp: 2024-10-23T01:59:27.215Z
Learning: In `tests/basic_integration/test.sh`, the hardcoded `CIPHERNODE_SECRET` is acceptable for testing purposes and does not need to be changed.
Applied to files:
deploy/local/contracts.sh
📚 Learning: 2025-10-04T14:18:45.636Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 660
File: crates/support-scripts/src/program_dev.rs:17-18
Timestamp: 2025-10-04T14:18:45.636Z
Learning: In crates/support-scripts/src/program_dev.rs, the #[allow(dead_code)] attribute on ProgramSupportDev is intentionally kept even though the struct appears to be referenced elsewhere, as confirmed by the maintainer.
Applied to files:
crates/support-scripts/src/lib.rscrates/support-scripts/src/program_dev.rs
📚 Learning: 2024-10-29T01:03:50.414Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/config/src/app_config.rs:21-26
Timestamp: 2024-10-29T01:03:50.414Z
Learning: In `packages/ciphernode/config/src/app_config.rs`, the `rpc_url` field in the `ChainConfig` struct is not considered sensitive and does not need to be encrypted.
Applied to files:
crates/config/src/app_config.rs
📚 Learning: 2024-10-29T00:05:52.278Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/config/src/app_config.rs:133-144
Timestamp: 2024-10-29T00:05:52.278Z
Learning: In the `load_config` function in `packages/ciphernode/config/src/app_config.rs`, Figment ensures correct types for the final configuration from defaults, reducing the need for additional validation after loading.
Applied to files:
crates/config/src/app_config.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/config/src/app_config.rs
🧬 Code graph analysis (6)
crates/cli/src/program.rs (1)
crates/support-scripts/src/lib.rs (1)
program_upload(30-32)
crates/support-scripts/src/program.rs (3)
crates/support-scripts/src/program_dev.rs (1)
upload(37-43)crates/support-scripts/src/program_risc0.rs (1)
upload(62-79)crates/support-scripts/src/traits.rs (1)
upload(14-14)
crates/support-scripts/src/lib.rs (1)
crates/support-scripts/src/program.rs (1)
new(28-34)
crates/support-scripts/src/program_dev.rs (4)
crates/support-scripts/src/program.rs (1)
upload(52-57)crates/support-scripts/src/program_risc0.rs (1)
upload(62-79)crates/support-scripts/src/traits.rs (1)
upload(14-14)crates/support-scripts/src/utils.rs (2)
ensure_script_exists(42-47)run_bash_script(13-40)
crates/support/app/src/main.rs (2)
crates/program-server/src/lib.rs (3)
new(35-46)process_computation_background(197-236)call_webhook(138-181)crates/support/host/src/lib.rs (1)
run_compute(218-253)
crates/support/host/src/lib.rs (1)
crates/support/program/src/lib.rs (1)
fhe_processor(13-23)
🪛 Shellcheck (0.11.0)
deploy/local/nodes.sh
[warning] 15-15: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
[warning] 15-15: Did you forget to close this double quoted string?
(SC1078)
[warning] 16-16: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
⏰ 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). (10)
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (22)
crates/support/Cargo.toml (1)
23-23: I need to verify these dependencies with more direct queries. Let me try simpler crates.io API calls and search for compatibility information.Let me search for more specific information about the alloy-signer-local 1.0 release and the other dependencies.
Based on my verification, three of the four dependencies are confirmed to be current and compatible:
boundless-market 1.1.0 depends on alloy-primitives ^1.0 and alloy-sol-types ^1.0, confirming compatibility with the workspace's pinned versions (alloy-primitives and alloy-sol-types at 1.3.0).
dotenvy 0.15.7 is the current stable release, and it is the suggested alternative for dotenv in security advisory RUSTSEC-2021-0141.
url 2.5.4 is also recent, with version 2.5.7 available as a newer patch.
However, alloy-signer-local 1.0.23 remains unverified. While alloy-signer-local 1.0.23 exists on crates.io, documentation shows version 0.12.6 with incompatible dependencies (alloy-primitives ^0.8.22). The compatibility of 1.0.23 with alloy-primitives 1.3.0 could not be definitively confirmed.
deploy/local/contracts.sh (1)
7-7: Approve contract deployment and ciphernode enrollment updates.The changes correctly remove the mock verifier flag for real Boundless deployment and add CN4/CN5 ciphernodes to the enclave. The new addresses (0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65 for CN4 and 0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc for CN5) properly correspond to the wallet keys set up in
nodes.sh, maintaining consistency across the deployment pipeline.Please verify that after fixing the quote escaping in
deploy/local/nodes.sh, the wallet initialization for cn4 and cn5 completes successfully and the nodes come up without errors.Also applies to: 16-24
examples/CRISP/server/src/server/models.rs (1)
16-16: LGTM! Optional deserializer handles missing data gracefully.The switch to
deserialize_hex_string_optionalwithdefaultforciphertextandproofcorrectly handles cases where these fields may be absent (e.g., on computation failure). The implementation properly handles None values and 0x prefix stripping.Also applies to: 19-19, 35-47
examples/CRISP/server/src/server/routes/state.rs (4)
44-46: LGTM! Enhanced logging improves observability.The additional context (status, ciphertext length, proof length) will help with debugging and monitoring computation outcomes.
68-75: LGTM! Unknown status handling is appropriate.The defensive check for unknown status values is good practice and returns an appropriate
BadRequestresponse. This validation would become unnecessary ifstatuswere changed to an enum (as suggested in models.rs).
98-98: LGTM! Appropriate log level for contract creation failure.Changing from
infotoerrorcorrectly reflects the severity of a contract creation failure.
77-86: Document the intended behavior for empty ciphertext/proof on completed status.The code intentionally accepts
ciphertextandproofas empty when status is "completed" (via optional deserialization fields and the guard at line 77). However, this behavior is undocumented and untested.Before accepting this, clarify:
- Is an empty result a valid "completed" state? If so, under what computation scenarios?
- What is the impact of skipping chain publication when both are empty?
- Should this condition emit a warning or metric to detect unexpected occurrences?
- Why don't the zk-inputs tests (which assert non-empty ciphertext) contradict this?
Consider adding:
- A test case covering this scenario
- A code comment explaining when/why this occurs
- Possibly stricter validation (e.g., require at least one field if status is "completed")
crates/support/scripts/container/start.sh (1)
4-55: Boundless env wiring and dev-mode fallback look consistentThe updated script cleanly:
- Resets Boundless-related env vars per invocation.
- Maps new CLI flags to
RISC0_DEV_MODE,RPC_URL,PRIVATE_KEY,PINATA_JWT,PROGRAM_URL,BOUNDLESS_ONCHAIN.- Defaults to
RISC0_DEV_MODE=1only when noRPC_URLis provided, which matches the host-side check (RISC0_DEV_MODEunset ⇒ production by default, so an explicit1is only needed for dev).Looks good for the new Boundless workflow.
crates/support-scripts/src/traits.rs (1)
14-14: LGTM!The new
uploadmethod follows the established trait pattern and integrates cleanly with the existingcompileandstartmethods.crates/support-scripts/src/program.rs (1)
52-57: LGTM!The
uploadmethod implementation correctly delegates to the appropriate variant and follows the same pattern ascompileandstart.crates/support-scripts/src/program_dev.rs (1)
37-42: LGTM!The
uploadimplementation follows the established pattern for dev mode. Using the sharedctl/uploadpath (rather thandev/upload) is appropriate since upload functionality is common across both dev and production modes.crates/support-scripts/src/lib.rs (1)
29-32: LGTM!The
program_uploadfunction provides a clean public API that's consistent with the existingprogram_compileandprogram_startfunctions. The documentation clearly explains its purpose.templates/default/enclave.config.yaml (2)
24-24: Improved documentation clarity.The updated comment better explains the semantics of
risc0_dev_modein the context of the Boundless migration.
26-32: Well-structured configuration template.The commented Boundless configuration block provides clear guidance for production setup. The reminder to use environment variables for secrets is a helpful security note.
examples/CRISP/enclave.config.yaml (2)
6-6: Verify the e3_program address change is intentional.The e3_program address has changed to match the default template. Please confirm this reflects a new deployment or intentional update for the Boundless migration.
22-29: Good addition of Boundless configuration example.The commented configuration provides a concrete example with an actual IPFS CID, which helps users understand the expected format.
crates/program-server/src/types.rs (3)
27-32: LGTM!The
ComputationStatusenum is well-designed with appropriate derives. The lowercase serialization format is clean and user-friendly.
38-38: LGTM!The addition of
statusand optionalerrorfields enhances webhook payload expressiveness. Usingskip_serializing_iffor the error field keeps the JSON clean when there's no error.Also applies to: 45-46
84-84: LGTM!The test updates properly validate the serialization of the new
statusanderrorfields, confirming the lowercase format works as expected.Also applies to: 158-159, 163-163
crates/support-scripts/src/program_risc0.rs (2)
37-59: LGTM with validation assumption.The Boundless configuration integration is well-structured. The code assumes
rpc_urlandprivate_keyare validated elsewhere in the config (non-empty when boundless is present), which is a reasonable assumption for this layer.
62-78: LGTM!The
uploadmethod correctly handles the optional nature of risc0 config, boundless config, and pinata_jwt through proper nested option checking. The implementation is clean and straightforward.crates/support/types/src/lib.rs (1)
26-31: ComputationStatus and extended WebhookPayload shape look good
- Enum derives and
#[serde(rename_all = "lowercase")]are appropriate for a simple"completed"/"failed"API surface.- Adding
statusand optionalerroronWebhookPayloadis a straightforward way to make webhook consumers status-aware without forcing an error field when there is none.Also applies to: 36-43
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/CRISP/server/src/server/routes/state.rs (1)
44-55: Use an error HTTP status forWebhookPayload::Failedand track failure TODOs explicitlyReturning
HttpResponse::Ok()for a failed computation makes it difficult for clients and monitoring to distinguish success from failure, and the TODOs for state/rewards/on-chain handling remain unresolved in this path. Consider switching this response to a 5xx status (e.g.InternalServerError) and either implementing or externally tracking the failure-side responsibilities.Example minimal change:
- HttpResponse::Ok().json(format!( + HttpResponse::InternalServerError().json(format!( "Computation failed for E3 ID: {}. Error: {}", e3_id, error ))
🧹 Nitpick comments (1)
examples/CRISP/server/src/server/routes/state.rs (1)
56-120: Avoid cloning potentially large ciphertext/proof when building the contract call
Bytes::from(ciphertext.clone())andBytes::from(proof.clone())introduce an extra copy of what are likely large buffers; you can move theVec<u8>directly intoBytesinstead, since they aren’t used after the contract call.For example:
- let tx_result = contract - .publish_ciphertext_output( - U256::from(e3_id), - Bytes::from(ciphertext.clone()), - Bytes::from(proof.clone()), - ) + let tx_result = contract + .publish_ciphertext_output( + U256::from(e3_id), + Bytes::from(ciphertext), + Bytes::from(proof), + ) .await;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/program-server/src/lib.rs(2 hunks)crates/program-server/src/types.rs(2 hunks)examples/CRISP/server/src/server/models.rs(1 hunks)examples/CRISP/server/src/server/routes/state.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-12T10:24:07.572Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
examples/CRISP/server/src/server/routes/state.rs
🧬 Code graph analysis (1)
crates/program-server/src/lib.rs (1)
crates/support/app/src/main.rs (1)
call_webhook(18-61)
⏰ 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). (11)
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: Build & Push Image
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: crisp_rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
examples/CRISP/server/src/server/models.rs (1)
11-37: WebhookPayload enum shape and hex deserialization look consistent and soundThe
status-taggedCompleted/Failedenum with per-field hex deserialization for ciphertext and proof matches the program-server side and keeps logs sane viaDebug = "ignore"; this is a clean way to model success vs failure payloads.crates/program-server/src/lib.rs (3)
138-180: Webhook caller correctly uses the new enum payload and surfaces HTTP failures
call_webhookcleanly derivese3_idfrom the enum, logs per-variant details, posts the JSON payload, and returns an error enriched with the response body on non-success status, which is a solid pattern for debugging webhook issues.
182-187: Thinhandle_webhook_deliverywrapper keeps the call site simpleWrapping
call_webhookbehindhandle_webhook_deliverykeeps the background processor focused on computation logic while centralizing webhook behavior and logging.
195-220: Background computation now maps runner results to Completed/Failed webhook payloads appropriatelyThe match on
runner(fhe_inputs).awaitcorrectly buildsWebhookPayload::Completed { e3_id, ciphertext, proof }on success andWebhookPayload::Failed { e3_id, error }on failure, ensuring the webhook always receives an explicit status while still propagating the original compute error back to the spawner for logging.crates/program-server/src/types.rs (2)
27-46: Tagged WebhookPayload enum cleanly models success vs failure for webhook consumersThe
#[serde(tag = "status", rename_all = "lowercase")]enum withCompleted { e3_id, ciphertext, proof }andFailed { e3_id, error }, plus hex serialization for the byte fields, gives a clear and backward-compatible wire format for downstream services.
48-79: Hex (de)serialization helpers and tests solidify the payload contract
serialize_as_hex,deserialize_hex_string, anddeserialize_hex_tuplecorrectly handle optional0xprefixes and large payloads, and the new tests fortest_webhook_payload_serialization_completed/failedpin the exact JSON representation, reducing the risk of accidental breaking changes.Also applies to: 56-63
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.