Add vaultless field to OrderIOCfg with validation (Phase 1)#2411
Open
findolor wants to merge 16 commits into2025-07-11-direct-swapfrom
Open
Add vaultless field to OrderIOCfg with validation (Phase 1)#2411findolor wants to merge 16 commits into2025-07-11-direct-swapfrom
findolor wants to merge 16 commits into2025-07-11-direct-swapfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds vaultless support to order settings and parsing, enforces vault-id validation (disallow explicit zero and disallow vault-id when vaultless=true), updates AddOrderArgs to generate non-zero random vault IDs and treat vaultless as B256::ZERO, removes Sentry parsing, bumps spec version 4→5, and updates tests and remote URLs. Changes
Sequence Diagram(s)sequenceDiagram
participant YAML as YAML parser (settings)
participant Validator as Order config validator
participant AddOrder as AddOrderArgs constructor
participant RNG as RNG (vault id generator)
YAML->>Validator: parse OrderIOCfg (vault_id?, vaultless?)
Validator->>Validator: enforce rules:
Note right of Validator: - if vaultless==true && vault_id provided => error\n- if vault_id == 0 => error
Validator-->>YAML: parsed & validated OrderIOCfg
AddOrder->>Validator: receive validated IO configs
AddOrder->>AddOrder: for each IO entry:
alt vaultless == true
AddOrder->>AddOrder: set vault_id = B256::ZERO
else vault_id provided and != 0
AddOrder->>AddOrder: use provided vault_id
else
AddOrder->>RNG: generate_nonzero_random_vault_id()
RNG-->>AddOrder: non-zero B256
AddOrder->>AddOrder: use generated vault_id
end
AddOrder-->>...: produce AddOrderArgs with validated vault ids
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
findolor
requested review from
0xgleb and
hardyjosh
and removed request for
0xgleb
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/settings/src/order.rs`:
- Around line 698-731: The parsing change forbids providing a vault-id when
vaultless is true, so update the mutation helpers (populate_vault_ids and
update_vault_id) to respect that invariant: when processing an entry whose
vaultless flag is Some(true) skip inserting/overwriting vault-id (or return
YamlError::ParseOrderConfigSourceError(ParseOrderConfigSourceError::VaultIdNotAllowedInVaultlessMode)
if you prefer surfacing the error) instead of unconditionally setting a vault
id; locate logic that writes into OrderIOCfg.vault_id or manipulates the YAML
for vault-id and add a guard checking OrderIOCfg.vaultless (or the parsed
"vaultless" YAML field) before mutating, mirroring the validation in
OrderCfg::validate_vault_id.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/common/src/add_order.rs`:
- Around line 124-131: The code currently treats vaultless == true as silently
overriding any provided vault_id; instead, add a guard in the vault_id
computation in add_order.rs to reject the combination: if input.vaultless ==
Some(true) and input.vault_id.is_some() (and not equal to U256::ZERO) return a
new/appropriate error (e.g.,
AddOrderArgsError::ConflictingVaultlessAndVaultId(i)) rather than proceeding;
apply the same guard to the similar block around lines 145–151 to ensure both
places validate this conflicting configuration before computing vault_id.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/settings/src/order.rs (1)
90-115: Rejectvault-id: 0in update_vault_id to match new validation.Line 94 currently accepts
0and writes an invalid config, bypassing the new “Invalid vault-id value” rule enforced in parsing. Add a zero check here to keep mutation helpers consistent with validation.🛠️ Proposed fix
- match OrderCfg::validate_vault_id(v) { - Ok(id) => Some(id), + match OrderCfg::validate_vault_id(v) { + Ok(id) => { + if id == U256::ZERO { + return Err(YamlError::ParseOrderConfigSourceError( + ParseOrderConfigSourceError::InvalidVaultIdZero, + )); + } + Some(id) + } Err(e) => { return Err(YamlError::Field { kind: FieldErrorKind::InvalidValue { field: "vault-id".to_string(), reason: e.to_string(), }, location: format!(
Collaborator
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/add_order.rs (1)
6-8: Conditional import ofU256conflicts with unconditional usage in public method.
U256is imported only under#[cfg(not(target_family = "wasm"))](line 8), but this public async method usesU256::ZEROon lines 133 and 157 without cfg guards. Sincenew_from_deploymentis public and callable from WASM contexts (js_api calls it without cfg), this creates a mismatch. ImportU256unconditionally to resolve the inconsistency and prevent potential WASM build issues.Suggested fix
-use alloy::primitives::{hex::FromHexError, Address, B256}; -#[cfg(not(target_family = "wasm"))] -use alloy::primitives::{FixedBytes, U256}; +use alloy::primitives::{hex::FromHexError, Address, B256, U256}; +#[cfg(not(target_family = "wasm"))] +use alloy::primitives::FixedBytes;
4 tasks
case-insensitive boolean parsing that accepts true/false/1/0
- add vaultless: Option<bool> field to OrderIOCfg - error if vaultless: true AND vault-id both specified - error if vault-id: 0 (use vaultless: true instead) - add tests for vaultless parsing and validation
- use B256::ZERO for vault_id when vaultless is true - ensure random vault id generation never produces zero
- use optional_bool for show-custom-field in gui.rs - simplify get_sentry() to use optional_bool - remove unused sentry.rs module
Prevent update_vault_id and populate_vault_ids from inserting vault IDs into vaultless entries, which would violate the new validation rules.
Defense-in-depth for programmatic DeploymentCfg construction that bypasses YAML parsing validation. Also aligns error messages with the settings crate style.
…s for random generation - validate_vault_id now checks for zero and returns InvalidVaultIdZero error - update_vault_id now properly rejects zero values through validate_vault_id - populate_vault_ids uses inline loop to ensure non-zero random vault IDs - removed generate_nonzero_random_vault_id helper in favor of inline loop - added test for update_vault_id rejecting zero values
findolor
force-pushed
the
2025-01-20-vaultless-phase-1-settings
branch
from
c8cf283 to
b14a237
Compare
Collaborator
Author
|
After rainlanguage/rainix#101 is merged, run Issue: nixpkgs updated wasm-bindgen-cli to 0.2.108, but rain.wasm requires 0.2.100. The rainix PR pins it back to 0.2.100. |
- Update take_orders.rs to use IOrderBookV6 (V5 no longer exists) - Point flake to rainix branch that pins wasm-bindgen-cli to 0.2.100
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dependent PRs
Motivation
See issues:
This PR implements Phase 1 of vaultless order support, adding the settings and validation layer. Vaultless orders allow users to place orders directly from their wallet without using the orderbook's vault system (vault_id = 0).
Solution
Core Changes
vaultless: Option<bool>field toOrderIOCfgstructoptional_boolhelper function for YAML parsing (case-insensitive, accepts true/false/1/0)vaultless: true→ Usesvault_id = 0(wallet)vaultless: true+vault-id→ Error: "Using vault-id is not allowed in vaultless mode"vault-id: 0→ Error: "Invalid vault-id value. For vaultless mode use vaultless: true"Refactoring
get_sentry()to useoptional_booloptional_boolforshow-custom-fieldin GUIsentry.rsmoduleBreaking Change
Checks
By submitting this for review, I'm confirming I've done the following:
fix #2403
Summary by CodeRabbit
New Features
Chores
Behavior / Validation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.