Add vaultless order IO settings#2769
Conversation
How to use the Graphite Merge QueueAdd the label Raindex-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughAdds a ChangesVaultless OrderIOCfg support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
crates/settings/src/order.rs (1)
292-405: 💤 Low valueConsider extracting shared YAML traversal logic.
update_vaultlessandupdate_vault_idshare nearly identical YAML document traversal and error handling patterns. Extracting the navigation into a helper that returns a mutable reference to the targetitem_mapwould reduce ~60 lines of duplication.This is a low-priority refactor since both methods work correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/settings/src/order.rs` around lines 292 - 405, Extract the common YAML traversal logic shared between update_vaultless and update_vault_id methods into a private helper method. Create a helper that takes vault_type, token, and returns a mutable reference to the target item_map hash within the document structure (navigating through document -> orders -> order -> inputs/outputs array -> matching item). This helper should handle all the nested option unwrapping and error cases for document, orders, order, and array access. Then refactor both update_vaultless and update_vault_id to call this helper instead of duplicating the ~60 lines of traversal and error handling code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/settings/src/order.rs`:
- Around line 292-405: Extract the common YAML traversal logic shared between
update_vaultless and update_vault_id methods into a private helper method.
Create a helper that takes vault_type, token, and returns a mutable reference to
the target item_map hash within the document structure (navigating through
document -> orders -> order -> inputs/outputs array -> matching item). This
helper should handle all the nested option unwrapping and error cases for
document, orders, order, and array access. Then refactor both update_vaultless
and update_vault_id to call this helper instead of duplicating the ~60 lines of
traversal and error handling code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22a7cbea-8db9-4e71-86b1-03ceee2a4f18
📒 Files selected for processing (3)
crates/common/src/add_order.rscrates/settings/src/order.rscrates/settings/src/yaml/context.rs
| pub struct OrderIOCfg { | ||
| pub token_key: String, | ||
| pub token: Option<Arc<TokenCfg>>, | ||
| #[serde(default)] |
There was a problem hiding this comment.
should fix: vaultless: bool alongside vault_id: Option<U256> lets the type hold the illegal vaultless = true, vault_id = Some(..) state, which you then rule out at runtime in parse_io_vault_fields and guard procedurally in both update helpers. With #[serde(default)] a deserialize can also bypass the validated parse path. A small sum type like enum VaultConfig { Vaultless, WithId(U256), Unset } makes the illegal combo unrepresentable, deletes the runtime guard, and removes the manual vaultless: false you now write at every struct literal.
| let mut document = self | ||
| .document | ||
| .write() | ||
| .map_err(|_| YamlError::WriteLockError)?; | ||
|
|
||
| if let StrictYaml::Hash(ref mut document_hash) = *document { | ||
| if let Some(StrictYaml::Hash(ref mut orders)) = | ||
| document_hash.get_mut(&StrictYaml::String("orders".to_string())) | ||
| { | ||
| if let Some(StrictYaml::Hash(ref mut order)) = | ||
| orders.get_mut(&StrictYaml::String(self.key.to_string())) | ||
| { | ||
| let vec_key = match vault_type { | ||
| VaultType::Input => "inputs", | ||
| VaultType::Output => "outputs", | ||
| }; | ||
| if let Some(StrictYaml::Array(ref mut vec)) = | ||
| order.get_mut(&StrictYaml::String(vec_key.to_string())) | ||
| { | ||
| let item_index = vec.iter().position(|item| { | ||
| if let StrictYaml::Hash(ref item_map) = item { | ||
| if let Some(StrictYaml::String(item_token)) = | ||
| item_map.get(&StrictYaml::String("token".to_string())) | ||
| { | ||
| return item_token == &token; | ||
| } | ||
| } | ||
| false | ||
| }); | ||
|
|
||
| if let Some(idx) = item_index { | ||
| if let Some(item) = vec.get_mut(idx) { | ||
| if let StrictYaml::Hash(ref mut item_map) = item { | ||
| if vaultless { | ||
| item_map.insert( | ||
| StrictYaml::String("vaultless".to_string()), | ||
| StrictYaml::String("true".to_string()), | ||
| ); | ||
| item_map | ||
| .remove(&StrictYaml::String("vault-id".to_string())); | ||
| } else { | ||
| item_map | ||
| .remove(&StrictYaml::String("vaultless".to_string())); | ||
| } | ||
|
|
||
| match vault_type { | ||
| VaultType::Input => { | ||
| self.inputs[idx].vaultless = vaultless; | ||
| if vaultless { | ||
| self.inputs[idx].vault_id = None; | ||
| } | ||
| } | ||
| VaultType::Output => { | ||
| self.outputs[idx].vaultless = vaultless; | ||
| if vaultless { | ||
| self.outputs[idx].vault_id = None; | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::InvalidType { | ||
| field: vec_key.to_string(), | ||
| expected: "a hash".to_string(), | ||
| }, | ||
| location: format!("order '{0}'", self.key), | ||
| }); | ||
| } | ||
| } | ||
| } else { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::InvalidValue { | ||
| field: vec_key.to_string(), | ||
| reason: format!("token '{}' not found", token), | ||
| }, | ||
| location: format!("order '{0}'", self.key), | ||
| }); | ||
| } | ||
| } else { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::Missing(vec_key.to_string()), | ||
| location: format!("order '{0}'", self.key), | ||
| }); | ||
| } | ||
| } else { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::Missing(self.key.clone()), | ||
| location: "orders".to_string(), | ||
| }); | ||
| } | ||
| } else { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::Missing("orders".to_string()), | ||
| location: "root".to_string(), | ||
| }); | ||
| } | ||
| } else { | ||
| return Err(YamlError::Field { | ||
| kind: FieldErrorKind::InvalidType { | ||
| field: "document".to_string(), | ||
| expected: "a map".to_string(), | ||
| }, | ||
| location: "root".to_string(), | ||
| }); | ||
| } | ||
|
|
||
| Ok(self.clone()) |
There was a problem hiding this comment.
minor: the body here is a six-level if let pyramid where each level's else is an error return, which is hard to read. Flattening it with let ... else early returns is much cleaner:
| let mut document = self | |
| .document | |
| .write() | |
| .map_err(|_| YamlError::WriteLockError)?; | |
| if let StrictYaml::Hash(ref mut document_hash) = *document { | |
| if let Some(StrictYaml::Hash(ref mut orders)) = | |
| document_hash.get_mut(&StrictYaml::String("orders".to_string())) | |
| { | |
| if let Some(StrictYaml::Hash(ref mut order)) = | |
| orders.get_mut(&StrictYaml::String(self.key.to_string())) | |
| { | |
| let vec_key = match vault_type { | |
| VaultType::Input => "inputs", | |
| VaultType::Output => "outputs", | |
| }; | |
| if let Some(StrictYaml::Array(ref mut vec)) = | |
| order.get_mut(&StrictYaml::String(vec_key.to_string())) | |
| { | |
| let item_index = vec.iter().position(|item| { | |
| if let StrictYaml::Hash(ref item_map) = item { | |
| if let Some(StrictYaml::String(item_token)) = | |
| item_map.get(&StrictYaml::String("token".to_string())) | |
| { | |
| return item_token == &token; | |
| } | |
| } | |
| false | |
| }); | |
| if let Some(idx) = item_index { | |
| if let Some(item) = vec.get_mut(idx) { | |
| if let StrictYaml::Hash(ref mut item_map) = item { | |
| if vaultless { | |
| item_map.insert( | |
| StrictYaml::String("vaultless".to_string()), | |
| StrictYaml::String("true".to_string()), | |
| ); | |
| item_map | |
| .remove(&StrictYaml::String("vault-id".to_string())); | |
| } else { | |
| item_map | |
| .remove(&StrictYaml::String("vaultless".to_string())); | |
| } | |
| match vault_type { | |
| VaultType::Input => { | |
| self.inputs[idx].vaultless = vaultless; | |
| if vaultless { | |
| self.inputs[idx].vault_id = None; | |
| } | |
| } | |
| VaultType::Output => { | |
| self.outputs[idx].vaultless = vaultless; | |
| if vaultless { | |
| self.outputs[idx].vault_id = None; | |
| } | |
| } | |
| } | |
| } else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::InvalidType { | |
| field: vec_key.to_string(), | |
| expected: "a hash".to_string(), | |
| }, | |
| location: format!("order '{0}'", self.key), | |
| }); | |
| } | |
| } | |
| } else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::InvalidValue { | |
| field: vec_key.to_string(), | |
| reason: format!("token '{}' not found", token), | |
| }, | |
| location: format!("order '{0}'", self.key), | |
| }); | |
| } | |
| } else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::Missing(vec_key.to_string()), | |
| location: format!("order '{0}'", self.key), | |
| }); | |
| } | |
| } else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::Missing(self.key.clone()), | |
| location: "orders".to_string(), | |
| }); | |
| } | |
| } else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::Missing("orders".to_string()), | |
| location: "root".to_string(), | |
| }); | |
| } | |
| } else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::InvalidType { | |
| field: "document".to_string(), | |
| expected: "a map".to_string(), | |
| }, | |
| location: "root".to_string(), | |
| }); | |
| } | |
| Ok(self.clone()) | |
| let mut document = self | |
| .document | |
| .write() | |
| .map_err(|_| YamlError::WriteLockError)?; | |
| let StrictYaml::Hash(ref mut document_hash) = *document else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::InvalidType { | |
| field: "document".to_string(), | |
| expected: "a map".to_string(), | |
| }, | |
| location: "root".to_string(), | |
| }); | |
| }; | |
| let Some(StrictYaml::Hash(ref mut orders)) = | |
| document_hash.get_mut(&StrictYaml::String("orders".to_string())) | |
| else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::Missing("orders".to_string()), | |
| location: "root".to_string(), | |
| }); | |
| }; | |
| let Some(StrictYaml::Hash(ref mut order)) = | |
| orders.get_mut(&StrictYaml::String(self.key.to_string())) | |
| else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::Missing(self.key.clone()), | |
| location: "orders".to_string(), | |
| }); | |
| }; | |
| let vec_key = match vault_type { | |
| VaultType::Input => "inputs", | |
| VaultType::Output => "outputs", | |
| }; | |
| let Some(StrictYaml::Array(ref mut vec)) = | |
| order.get_mut(&StrictYaml::String(vec_key.to_string())) | |
| else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::Missing(vec_key.to_string()), | |
| location: format!("order '{0}'", self.key), | |
| }); | |
| }; | |
| let item_index = vec.iter().position(|item| { | |
| if let StrictYaml::Hash(ref item_map) = item { | |
| if let Some(StrictYaml::String(item_token)) = | |
| item_map.get(&StrictYaml::String("token".to_string())) | |
| { | |
| return item_token == &token; | |
| } | |
| } | |
| false | |
| }); | |
| let Some(idx) = item_index else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::InvalidValue { | |
| field: vec_key.to_string(), | |
| reason: format!("token '{}' not found", token), | |
| }, | |
| location: format!("order '{0}'", self.key), | |
| }); | |
| }; | |
| let Some(StrictYaml::Hash(ref mut item_map)) = vec.get_mut(idx) else { | |
| return Err(YamlError::Field { | |
| kind: FieldErrorKind::InvalidType { | |
| field: vec_key.to_string(), | |
| expected: "a hash".to_string(), | |
| }, | |
| location: format!("order '{0}'", self.key), | |
| }); | |
| }; | |
| if vaultless { | |
| item_map.insert( | |
| StrictYaml::String("vaultless".to_string()), | |
| StrictYaml::String("true".to_string()), | |
| ); | |
| item_map.remove(&StrictYaml::String("vault-id".to_string())); | |
| } else { | |
| item_map.remove(&StrictYaml::String("vaultless".to_string())); | |
| } | |
| let io = match vault_type { | |
| VaultType::Input => &mut self.inputs[idx], | |
| VaultType::Output => &mut self.outputs[idx], | |
| }; | |
| io.vaultless = vaultless; | |
| if vaultless { | |
| io.vault_id = None; | |
| } | |
| Ok(self.clone()) |
Separately, this entire lock-and-descend skeleton is a verbatim copy of update_vault_id; only the leaf mutation differs. A shared private helper that locates the item map and index (e.g. with_io_item(vault_type, &token, |item_map, idx| ...)) would let both methods collapse to just their leaf write, though that is a bigger change touching update_vault_id too. Worth a compile check after applying the suggestion.
| ) | ||
| }; | ||
|
|
||
| if vaultless && vault_id.is_some() { |
There was a problem hiding this comment.
minor: this conflict check runs after validate_vault_id has already rejected zero, so vaultless: true together with vault-id: 0 reports ZeroVaultId ("use vaultless: true for vaultless IO"), which is confusing advice when the user already set vaultless. Move this check above the vault-id parse, or detect both keys present up front and return the conflict error first.
| .inputs | ||
| .get(index) | ||
| .map(|input| input.vaultless) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
minor: if the YAML inputs array is ever longer than self.inputs, get(index) is None and unwrap_or(false) treats the entry as non-vaultless, injecting a vault-id into what may be a vaultless IO with no error. It can't happen in the normal post-parse flow where lengths match, but an explicit error on mismatch (.get(index).ok_or_else(...)?.vaultless) turns a silent wrong result into a loud one. Same pattern applies to the outputs loop just below.
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_rejects_zero_vault_id() { |
There was a problem hiding this comment.
nit: this rejects vault-id: 0 only on the input side. parse_io_vault_fields runs for outputs too, so a matching case with vault-id: 0 in output_extra (asserting the output index location) would cover the other call site cheaply.
3 participants
Summary
vault-id: vaultlessas the authored settings sentinel for vaultless order IO.vault-id: 0so vaultless mode must be requested explicitly with thevaultlesssentinel.VaultConfiginternally so vaultless and numeric vault IDs are mutually exclusive.Dependent PRs
Validation
cargo check -p raindex_app_settings -p raindex_commoncargo test -p raindex_app_settings vaultlesscargo test -p raindex_app_settings zero_vault_idcargo test -p raindex_app_settings zero_output_vault_idcargo test -p raindex_app_settings update_vault_idcargo test -p raindex_common add_orderReview
Refs RAI-1063.
Summary by CodeRabbit
New Features
Bug Fixes
Chores