Conversation
There was a problem hiding this comment.
Pull request overview
Refactors deposit-address tracking to go through a storage abstraction (backed by an in-memory store for now), preparing for future registry-driven deposits.
Changes:
- Introduce
Storagetrait plus in-memorySharedStoreimplementation to hold monitored deposits and registry cursor state. - Move
MonitoredDepositmodel out ofdeposit_monitorintosrc/storage/model.rsand switchDepositMonitorto query storage at runtime. - Update CLI/output and test config/docs to align with the new monitored-deposit model and reclaim script parsing.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/solo/spox.toml | Sets valid hex reclaim_script values for test deposits. |
| tests/README.md | Updates local demo instructions for funding signers. |
| src/storage/model.rs | Adds monitored-deposit data model + config conversion + scriptPubKey derivation. |
| src/storage/mod.rs | Defines Storage trait API for monitored deposits + registry cursor. |
| src/storage/memory.rs | Implements Storage via an in-memory Arc<Mutex<...>> store. |
| src/main.rs | Loads monitored deposits into storage and updates address-printing to use source. |
| src/lib.rs | Exposes new storage module from the library. |
| src/error.rs | Adds PoisonedMutex error used by in-memory storage. |
| src/deposit_monitor.rs | Switches monitoring logic from an internal map to storage lookups. |
| src/context.rs | Adds shared storage (and settings) to Context and exposes accessors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug, Clone, Default)] | ||
| pub struct Store { | ||
| last_next_address_id: u64, | ||
| monitored: HashMap<ScriptBuf, MonitoredDeposit>, | ||
| } |
There was a problem hiding this comment.
Store derives Clone, which will deep-clone the full HashMap<ScriptBuf, MonitoredDeposit> if it’s ever used, potentially causing unintended copies and extra allocations. Since callers should clone SharedStore (the Arc) instead, consider removing Clone from Store to prevent accidental expensive clones.
| impl Storage for SharedStore { | ||
| /// Add a monitored deposit. If the script pubkey already exists it's a nop. | ||
| fn add(&self, monitored_deposit: MonitoredDeposit) -> Result<(), Error> { | ||
| let mut store = self.lock().map_err(|_| Error::PoisonedMutex)?; | ||
|
|
||
| let key = monitored_deposit.to_script_pubkey(); | ||
| store.monitored.entry(key).or_insert(monitored_deposit); | ||
| Ok(()) |
There was a problem hiding this comment.
The new in-memory Storage implementation introduces core behavior (deduping in add, correctness of get_scripts, and tracking last_next_address_id) but there are no unit tests covering it. Adding tests would help catch regressions as more storage backends and registry integration are introduced.
| )?, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
MonitoredDeposit::try_from contains non-trivial conversion logic (building DepositScriptInputs and validating ReclaimScriptInputs). Consider adding unit tests for this conversion (including invalid reclaim scripts / locktimes) so config parsing failures surface in a controlled way.
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| use bitcoin::opcodes::all::OP_TRUE; | |
| use bitcoin::script::Builder; | |
| use bitcoin::XOnlyPublicKey; | |
| use clarity::vm::types::PrincipalData; | |
| fn monitored_deposit_config(lock_time: u32, reclaim_script: ScriptBuf) -> MonitoredDepositConfig { | |
| MonitoredDepositConfig { | |
| signers_xonly: XOnlyPublicKey::from_str( | |
| "79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", | |
| ) | |
| .unwrap(), | |
| recipient: PrincipalData::parse_standard_principal("ST000000000000000000002AMW42H") | |
| .unwrap(), | |
| max_fee: 1_000, | |
| lock_time, | |
| reclaim_script, | |
| } | |
| } | |
| #[test] | |
| fn try_from_builds_monitored_deposit_for_valid_config() { | |
| let alias = "test-deposit".to_string(); | |
| let reclaim_script = Builder::new().push_opcode(OP_TRUE).into_script(); | |
| let config = monitored_deposit_config(500, reclaim_script.clone()); | |
| let monitored = MonitoredDeposit::try_from((&alias, &config)).unwrap(); | |
| match monitored.source { | |
| MonitoredDepositSource::Config(source_alias) => assert_eq!(source_alias, alias), | |
| MonitoredDepositSource::Registry(_) => panic!("expected config source"), | |
| } | |
| assert_eq!( | |
| monitored.deposit_script_inputs.signers_public_key, | |
| config.signers_xonly | |
| ); | |
| assert_eq!(monitored.deposit_script_inputs.recipient, config.recipient); | |
| assert_eq!(monitored.deposit_script_inputs.max_fee, config.max_fee); | |
| assert_eq!( | |
| monitored.reclaim_script_inputs.reclaim_script(), | |
| reclaim_script.as_script() | |
| ); | |
| } | |
| #[test] | |
| fn try_from_returns_error_for_invalid_reclaim_script() { | |
| let alias = "test-deposit".to_string(); | |
| let config = monitored_deposit_config(500, ScriptBuf::new()); | |
| let result = MonitoredDeposit::try_from((&alias, &config)); | |
| assert!(result.is_err()); | |
| } | |
| #[test] | |
| fn try_from_returns_error_for_invalid_lock_time() { | |
| let alias = "test-deposit".to_string(); | |
| let reclaim_script = Builder::new().push_opcode(OP_TRUE).into_script(); | |
| let config = monitored_deposit_config(0, reclaim_script); | |
| let result = MonitoredDeposit::try_from((&alias, &config)); | |
| assert!(result.is_err()); | |
| } | |
| } |
| &self.emily_config | ||
| } | ||
|
|
||
| /// Get a reference to the storage |
There was a problem hiding this comment.
The docstring says this returns “a reference to the storage”, but the function returns a cloned SharedStore handle (Arc<Mutex<...>>). Consider rewording to avoid implying a borrowed reference (e.g., “Get a handle to the shared storage”).
| /// Get a reference to the storage | |
| /// Get a handle to the shared storage |
Refactor to use a storage abstraction for the deposit addresses. We will add registered deposits from the contract in the storage in another PR.
Eventually we will use a proper storage, but for now a naive in-memory thingy should work just fine.
Testing information
Tested locally used the testing instruction from
tests/README.md, the whole flow was also tested locally with spox registry aware + front end (coming soon™)