fix(ui): prevent contract chooser from expanding multiple contracts on single click#616
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
thepastaclaw
force-pushed
the
fix/contract-chooser-expand-collision
branch
from
61cbf58 to
3917331
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/components/contract_chooser_panel.rs`:
- Around line 239-241: The closure body passed to ui.push_id(&contract_id, |ui|
{ is mis-indented compared to other closures (e.g., .show(ctx, |ui| {,
egui::ScrollArea::vertical().show(|ui| {, ui.vertical_centered(|ui| {)); fix by
reformatting the file so the block inside ui.push_id(&contract_id, |ui| { has
the same indentation style as the other closures and then run cargo +nightly fmt
on the file (or the whole repo) to apply Rustfmt rules and ensure consistent
indentation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/context/contract_token_db.rs (1)
16-61:⚠️ Potential issue | 🟠 MajorThe API design is misleading —
limitandoffsetonly apply to database results, not the final returned listAll call sites in the codebase pass
(None, None)for both parameters, so this is not a runtime issue today. However, the function signature and documentation are misleading: callers would reasonably expectlimitandoffsetto constrain the final result set (including the 5 prepended system contracts), but they only constrain database results.Either remove these parameters entirely, or fix the implementation to honor them for the complete result set. If keeping them, update the docstring to explicitly document that limit/offset apply only to database portion and system contracts are always included regardless.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/contract_token_db.rs` around lines 16 - 61, get_contracts currently applies limit/offset only to DB results, which is misleading; update get_contracts to apply limit and offset to the final list (system_contracts + db contracts) by building the combined result (currently variable result), then apply offset (skip) and limit (take) to that combined vector before returning, or if you prefer to keep the existing behavior, update the get_contracts docstring to clearly state that limit/offset apply only to database results and that system contracts (dpns_contract, token_history_contract, withdraws_contract, keyword_search_contract, dashpay_contract) are always prepended; make the change inside get_contracts so callers relying on the signature get the documented behavior.
🧹 Nitpick comments (1)
src/context/contract_token_db.rs (1)
27-45: RedundantArc::clone— use.as_ref().clone()directly
Arc::clone(&self.xxx_contract).as_ref().clone()needlessly bumps and immediately drops the Arc refcount. The simpler form is sufficient:♻️ Proposed refactor (apply to all five entries)
- contract: Arc::clone(&self.dpns_contract).as_ref().clone(), + contract: self.dpns_contract.as_ref().clone(),- contract: Arc::clone(&self.token_history_contract).as_ref().clone(), + contract: self.token_history_contract.as_ref().clone(),- contract: Arc::clone(&self.withdraws_contract).as_ref().clone(), + contract: self.withdraws_contract.as_ref().clone(),- contract: Arc::clone(&self.keyword_search_contract).as_ref().clone(), + contract: self.keyword_search_contract.as_ref().clone(),- contract: Arc::clone(&self.dashpay_contract).as_ref().clone(), + contract: self.dashpay_contract.as_ref().clone(),This also avoids a likely
clippy::redundant_clonewarning. As per coding guidelines,cargo clippyshould be run before finalising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/contract_token_db.rs` around lines 27 - 45, The QualifiedContract entries currently use Arc::clone(&self.<name>_contract).as_ref().clone(), which needlessly increments and drops the Arc; instead call as_ref().clone() directly on the Arc fields (e.g. self.dpns_contract.as_ref().clone(), self.token_history_contract.as_ref().clone(), self.withdraws_contract.as_ref().clone(), self.keyword_search_contract.as_ref().clone(), self.dashpay_contract.as_ref().clone()) so you clone the inner contract value without bumping the Arc refcount; update all five QualifiedContract initializers accordingly to remove the redundant Arc::clone call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/context/contract_token_db.rs`:
- Around line 16-61: get_contracts currently applies limit/offset only to DB
results, which is misleading; update get_contracts to apply limit and offset to
the final list (system_contracts + db contracts) by building the combined result
(currently variable result), then apply offset (skip) and limit (take) to that
combined vector before returning, or if you prefer to keep the existing
behavior, update the get_contracts docstring to clearly state that limit/offset
apply only to database results and that system contracts (dpns_contract,
token_history_contract, withdraws_contract, keyword_search_contract,
dashpay_contract) are always prepended; make the change inside get_contracts so
callers relying on the signature get the documented behavior.
---
Nitpick comments:
In `@src/context/contract_token_db.rs`:
- Around line 27-45: The QualifiedContract entries currently use
Arc::clone(&self.<name>_contract).as_ref().clone(), which needlessly increments
and drops the Arc; instead call as_ref().clone() directly on the Arc fields
(e.g. self.dpns_contract.as_ref().clone(),
self.token_history_contract.as_ref().clone(),
self.withdraws_contract.as_ref().clone(),
self.keyword_search_contract.as_ref().clone(),
self.dashpay_contract.as_ref().clone()) so you clone the inner contract value
without bumping the Arc refcount; update all five QualifiedContract initializers
accordingly to remove the redundant Arc::clone call.
3917331 to
77e225c
Compare
When a system contract (e.g. DPNS) was also added manually by the user, get_contracts() returned it twice: once from the DB (shown by Base58 ID) and once as the injected system contract (shown by alias). Both entries shared the same contract ID, so clicking either one toggled the same key in expanded_contracts, causing both to expand simultaneously. Fix: after fetching from the DB, filter out any contracts whose ID matches a system contract before prepending the system contracts.
thepastaclaw
force-pushed
the
fix/contract-chooser-expand-collision
branch
from
77e225c to
7e273aa
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK I think the title is a bit unclear; this fix is more "fix: deduplicate contract list when a system contract was manually added"
|
Addressed Lukasz’s feedback on contract dedup in Updated to use a contract-id keyed map ( Commit: c9db2ca6 |
|
@thepastaclaw claw you misunderstood my comment. Read whole function. See how you can benefit from BTreeMap or IndexMap. Like, get the result in lower number of lines. |
|
@lklimek Thanks — you were right, I had overcomplicated it. I re-read the whole What changed in
Commit pushed to this PR branch: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean UI fix that prevents contract chooser from expanding multiple contracts on a single click. The get_contracts dedup simplification is correct — preserves system-contract precedence and fixes the SQLite OFFSET-without-LIMIT edge case. No issues found.
Reviewed commit: ee9f490
|
✅ Review complete (commit ee9f490) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The fix correctly addresses the duplicate-Identifier collision in ContractChooserState.expanded_contracts. The dedup-source approach is sound. Concerns raised by reviewers about hiding 'newer' DB revisions of system contracts don't hold in practice: FetchContracts uses insert_contract_if_not_exists (never overwrites), and replace_contract is only invoked from user-owned-contract update flows — so DB copies of system contracts can only be older or equal to the bundled snapshot. The remaining valid concerns are latent pagination semantics, the u32::MAX fallback masking an invalid-SQL bug at the DB layer, missing test coverage, and a small idiom cleanup.
Reviewed commit: ee9f490
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/contract_token_db.rs`:
- [SUGGESTION] lines 24-67: Pagination is applied to raw DB rows, then dedup runs after — `limit`/`offset` will skew once duplicates exist
After this change, `get_contracts(Some(N), Some(M))` returns the 5 system contracts unconditionally plus a paged-then-deduped slice of DB rows. If a duplicate of a system contract falls inside the requested DB window, it consumes one of the `limit` slots and is then dropped, so callers receive fewer unique results than requested and items can be skipped across pages. All current call sites pass `(None, None)`, so this is latent — but the function now silently means `system ∪ paged(db)` rather than `paged(system ∪ db)`. Either gate the system-contract prepend on `offset.unwrap_or(0) == 0` and adjust `limit` to account for system count, or split into `get_system_contracts()` / `get_user_contracts(limit, offset)` so the distinction is encoded in the API rather than relying on the doc comment.
- [SUGGESTION] lines 29-30: `u32::MAX` fallback papers over an invalid-SQL bug in `database::contracts::get_contracts`
The `let db_limit = limit.or(offset.map(|_| u32::MAX));` exists because `database/contracts.rs::get_contracts` builds `... WHERE network = ? OFFSET ?` when `offset` is `Some` and `limit` is `None`, which SQLite rejects (LIMIT must precede OFFSET). Today every caller passes `(None, None)`, so this never fires, but the workaround entrenches a broken contract at the upper layer instead of fixing the actual bug at the DB layer (e.g. emit `LIMIT -1 OFFSET ?` or require both). Future direct callers of `db.get_contracts` will hit the same problem.
- [SUGGESTION] lines 24-67: No regression test for the dedup behavior
The bug being fixed is exactly the kind of regression a small unit test would catch: insert a contract whose ID equals `dpns_contract.id()` into the DB, call `get_contracts(None, None)`, and assert the result has 5 entries with the system aliases preserved. Without a test, future refactors of this function can silently re-introduce the duplicate-Identifier collision that broke `ContractChooserState.expanded_contracts`. Per the project's testing conventions, new logic should have inline `#[test]` coverage, and this is a small pure function ideal for that.
Fresh dispatcher-requested review for exact queued SHA ee9f490. Existing/same-SHA review dedup was intentionally bypassed per coordinator instruction.
| pub fn get_contracts( | ||
| &self, | ||
| limit: Option<u32>, | ||
| offset: Option<u32>, | ||
| ) -> Result<Vec<QualifiedContract>> { | ||
| // Get contracts from the database | ||
| let mut contracts = self.db.get_contracts(self, limit, offset)?; | ||
|
|
||
| // Add the DPNS contract to the list | ||
| let dpns_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.dpns_contract).as_ref().clone(), | ||
| alias: Some("dpns".to_string()), | ||
| }; | ||
|
|
||
| // Insert the DPNS contract at 0 | ||
| contracts.insert(0, dpns_contract); | ||
|
|
||
| // Add the token history contract to the list | ||
| let token_history_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.token_history_contract).as_ref().clone(), | ||
| alias: Some("token_history".to_string()), | ||
| }; | ||
|
|
||
| // Insert the token history contract at 1 | ||
| contracts.insert(1, token_history_contract); | ||
|
|
||
| // Add the withdrawal contract to the list | ||
| let withdraws_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.withdraws_contract).as_ref().clone(), | ||
| alias: Some("withdrawals".to_string()), | ||
| }; | ||
|
|
||
| // Insert the withdrawal contract at 2 | ||
| contracts.insert(2, withdraws_contract); | ||
|
|
||
| // Add the keyword search contract to the list | ||
| let keyword_search_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.keyword_search_contract).as_ref().clone(), | ||
| alias: Some("keyword_search".to_string()), | ||
| }; | ||
|
|
||
| // Insert the keyword search contract at 3 | ||
| contracts.insert(3, keyword_search_contract); | ||
|
|
||
| // Add the DashPay contract to the list | ||
| let dashpay_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.dashpay_contract).as_ref().clone(), | ||
| alias: Some("dashpay".to_string()), | ||
| }; | ||
|
|
||
| // Insert the DashPay contract at 4 | ||
| contracts.insert(4, dashpay_contract); | ||
|
|
||
| Ok(contracts) | ||
| let db_limit = limit.or(offset.map(|_| u32::MAX)); | ||
| let db_contracts = self.db.get_contracts(self, db_limit, offset)?; | ||
|
|
||
| let system_contracts = [ | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.dpns_contract).as_ref().clone(), | ||
| alias: Some("dpns".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.token_history_contract).as_ref().clone(), | ||
| alias: Some("token_history".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.withdraws_contract).as_ref().clone(), | ||
| alias: Some("withdrawals".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.keyword_search_contract).as_ref().clone(), | ||
| alias: Some("keyword_search".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.dashpay_contract).as_ref().clone(), | ||
| alias: Some("dashpay".to_string()), | ||
| }, | ||
| ]; | ||
|
|
||
| let mut seen_contract_ids = BTreeMap::new(); | ||
| Ok(system_contracts | ||
| .into_iter() | ||
| .chain(db_contracts) | ||
| .filter_map(|qualified_contract| { | ||
| let contract_id = qualified_contract.contract.id(); | ||
| seen_contract_ids | ||
| .insert(contract_id, ()) | ||
| .is_none() | ||
| .then_some(qualified_contract) | ||
| }) | ||
| .collect()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Pagination is applied to raw DB rows, then dedup runs after — limit/offset will skew once duplicates exist
After this change, get_contracts(Some(N), Some(M)) returns the 5 system contracts unconditionally plus a paged-then-deduped slice of DB rows. If a duplicate of a system contract falls inside the requested DB window, it consumes one of the limit slots and is then dropped, so callers receive fewer unique results than requested and items can be skipped across pages. All current call sites pass (None, None), so this is latent — but the function now silently means system ∪ paged(db) rather than paged(system ∪ db). Either gate the system-contract prepend on offset.unwrap_or(0) == 0 and adjust limit to account for system count, or split into get_system_contracts() / get_user_contracts(limit, offset) so the distinction is encoded in the API rather than relying on the doc comment.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/contract_token_db.rs`:
- [SUGGESTION] lines 24-67: Pagination is applied to raw DB rows, then dedup runs after — `limit`/`offset` will skew once duplicates exist
After this change, `get_contracts(Some(N), Some(M))` returns the 5 system contracts unconditionally plus a paged-then-deduped slice of DB rows. If a duplicate of a system contract falls inside the requested DB window, it consumes one of the `limit` slots and is then dropped, so callers receive fewer unique results than requested and items can be skipped across pages. All current call sites pass `(None, None)`, so this is latent — but the function now silently means `system ∪ paged(db)` rather than `paged(system ∪ db)`. Either gate the system-contract prepend on `offset.unwrap_or(0) == 0` and adjust `limit` to account for system count, or split into `get_system_contracts()` / `get_user_contracts(limit, offset)` so the distinction is encoded in the API rather than relying on the doc comment.
| let db_limit = limit.or(offset.map(|_| u32::MAX)); | ||
| let db_contracts = self.db.get_contracts(self, db_limit, offset)?; |
There was a problem hiding this comment.
🟡 Suggestion: u32::MAX fallback papers over an invalid-SQL bug in database::contracts::get_contracts
The let db_limit = limit.or(offset.map(|_| u32::MAX)); exists because database/contracts.rs::get_contracts builds ... WHERE network = ? OFFSET ? when offset is Some and limit is None, which SQLite rejects (LIMIT must precede OFFSET). Today every caller passes (None, None), so this never fires, but the workaround entrenches a broken contract at the upper layer instead of fixing the actual bug at the DB layer (e.g. emit LIMIT -1 OFFSET ? or require both). Future direct callers of db.get_contracts will hit the same problem.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/contract_token_db.rs`:
- [SUGGESTION] lines 29-30: `u32::MAX` fallback papers over an invalid-SQL bug in `database::contracts::get_contracts`
The `let db_limit = limit.or(offset.map(|_| u32::MAX));` exists because `database/contracts.rs::get_contracts` builds `... WHERE network = ? OFFSET ?` when `offset` is `Some` and `limit` is `None`, which SQLite rejects (LIMIT must precede OFFSET). Today every caller passes `(None, None)`, so this never fires, but the workaround entrenches a broken contract at the upper layer instead of fixing the actual bug at the DB layer (e.g. emit `LIMIT -1 OFFSET ?` or require both). Future direct callers of `db.get_contracts` will hit the same problem.
| let mut seen_contract_ids = BTreeMap::new(); | ||
| Ok(system_contracts | ||
| .into_iter() | ||
| .chain(db_contracts) | ||
| .filter_map(|qualified_contract| { | ||
| let contract_id = qualified_contract.contract.id(); | ||
| seen_contract_ids | ||
| .insert(contract_id, ()) | ||
| .is_none() | ||
| .then_some(qualified_contract) | ||
| }) | ||
| .collect()) |
There was a problem hiding this comment.
💬 Nitpick: Use HashSet/BTreeSet instead of BTreeMap<Identifier, ()> for the seen-id set
BTreeMap<Identifier, ()> is being used as a set — only the is_none() of insert is consumed. HashSet/BTreeSet::insert returns bool (true if newly inserted), giving a more direct expression and removing the dummy (). Identifier is a fixed-length byte array so HashSet hashes cheaply, and ordering isn't relied on (the chain order determines output order). Pure cleanup, no behavioral change.
💡 Suggested change
| let mut seen_contract_ids = BTreeMap::new(); | |
| Ok(system_contracts | |
| .into_iter() | |
| .chain(db_contracts) | |
| .filter_map(|qualified_contract| { | |
| let contract_id = qualified_contract.contract.id(); | |
| seen_contract_ids | |
| .insert(contract_id, ()) | |
| .is_none() | |
| .then_some(qualified_contract) | |
| }) | |
| .collect()) | |
| let mut seen_contract_ids = std::collections::HashSet::new(); | |
| Ok(system_contracts | |
| .into_iter() | |
| .chain(db_contracts) | |
| .filter(|qualified_contract| seen_contract_ids.insert(qualified_contract.contract.id())) | |
| .collect()) |
source: ['claude', 'codex']
| pub fn get_contracts( | ||
| &self, | ||
| limit: Option<u32>, | ||
| offset: Option<u32>, | ||
| ) -> Result<Vec<QualifiedContract>> { | ||
| // Get contracts from the database | ||
| let mut contracts = self.db.get_contracts(self, limit, offset)?; | ||
|
|
||
| // Add the DPNS contract to the list | ||
| let dpns_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.dpns_contract).as_ref().clone(), | ||
| alias: Some("dpns".to_string()), | ||
| }; | ||
|
|
||
| // Insert the DPNS contract at 0 | ||
| contracts.insert(0, dpns_contract); | ||
|
|
||
| // Add the token history contract to the list | ||
| let token_history_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.token_history_contract).as_ref().clone(), | ||
| alias: Some("token_history".to_string()), | ||
| }; | ||
|
|
||
| // Insert the token history contract at 1 | ||
| contracts.insert(1, token_history_contract); | ||
|
|
||
| // Add the withdrawal contract to the list | ||
| let withdraws_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.withdraws_contract).as_ref().clone(), | ||
| alias: Some("withdrawals".to_string()), | ||
| }; | ||
|
|
||
| // Insert the withdrawal contract at 2 | ||
| contracts.insert(2, withdraws_contract); | ||
|
|
||
| // Add the keyword search contract to the list | ||
| let keyword_search_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.keyword_search_contract).as_ref().clone(), | ||
| alias: Some("keyword_search".to_string()), | ||
| }; | ||
|
|
||
| // Insert the keyword search contract at 3 | ||
| contracts.insert(3, keyword_search_contract); | ||
|
|
||
| // Add the DashPay contract to the list | ||
| let dashpay_contract = QualifiedContract { | ||
| contract: Arc::clone(&self.dashpay_contract).as_ref().clone(), | ||
| alias: Some("dashpay".to_string()), | ||
| }; | ||
|
|
||
| // Insert the DashPay contract at 4 | ||
| contracts.insert(4, dashpay_contract); | ||
|
|
||
| Ok(contracts) | ||
| let db_limit = limit.or(offset.map(|_| u32::MAX)); | ||
| let db_contracts = self.db.get_contracts(self, db_limit, offset)?; | ||
|
|
||
| let system_contracts = [ | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.dpns_contract).as_ref().clone(), | ||
| alias: Some("dpns".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.token_history_contract).as_ref().clone(), | ||
| alias: Some("token_history".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.withdraws_contract).as_ref().clone(), | ||
| alias: Some("withdrawals".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.keyword_search_contract).as_ref().clone(), | ||
| alias: Some("keyword_search".to_string()), | ||
| }, | ||
| QualifiedContract { | ||
| contract: Arc::clone(&self.dashpay_contract).as_ref().clone(), | ||
| alias: Some("dashpay".to_string()), | ||
| }, | ||
| ]; | ||
|
|
||
| let mut seen_contract_ids = BTreeMap::new(); | ||
| Ok(system_contracts | ||
| .into_iter() | ||
| .chain(db_contracts) | ||
| .filter_map(|qualified_contract| { | ||
| let contract_id = qualified_contract.contract.id(); | ||
| seen_contract_ids | ||
| .insert(contract_id, ()) | ||
| .is_none() | ||
| .then_some(qualified_contract) | ||
| }) | ||
| .collect()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No regression test for the dedup behavior
The bug being fixed is exactly the kind of regression a small unit test would catch: insert a contract whose ID equals dpns_contract.id() into the DB, call get_contracts(None, None), and assert the result has 5 entries with the system aliases preserved. Without a test, future refactors of this function can silently re-introduce the duplicate-Identifier collision that broke ContractChooserState.expanded_contracts. Per the project's testing conventions, new logic should have inline #[test] coverage, and this is a small pure function ideal for that.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/contract_token_db.rs`:
- [SUGGESTION] lines 24-67: No regression test for the dedup behavior
The bug being fixed is exactly the kind of regression a small unit test would catch: insert a contract whose ID equals `dpns_contract.id()` into the DB, call `get_contracts(None, None)`, and assert the result has 5 entries with the system aliases preserved. Without a test, future refactors of this function can silently re-introduce the duplicate-Identifier collision that broke `ContractChooserState.expanded_contracts`. Per the project's testing conventions, new logic should have inline `#[test]` coverage, and this is a small pure function ideal for that.
3 participants
Issue
Clicking any contract in the Contracts chooser panel would also expand another contract. For example, clicking only the GWR contract would expand both DPNS and GWR simultaneously (and vice versa).
Root Cause
The GWR contract shown as
GWRSAVFMjXx8HpQFaNJMqBV7MBgMK4br5UESsB4S31Ecis the DPNS contract — that's DPNS's Base58-encoded contract ID.get_contracts()always injects system contracts (DPNS, DashPay, etc.) at fixed positions, but if the same contract was also added manually by the user (e.g. by fetching the DPNS contract by ID), it would exist in the database too. This resulted in two entries with the same contract ID but different display names — one showing "DPNS" (from the system alias) and one showing the raw Base58 ID (from the DB with no alias).Since the expand/collapse state in
ContractChooserState.expanded_contractsis keyed by contract ID, toggling either entry toggled the same key, causing both to expand or collapse simultaneously.Fix
Deduplicate contracts: After fetching from the DB, filter out any contracts whose ID matches a system contract before prepending the system contracts. This prevents duplicates.
Scope egui widget IDs: Wrap each contract's entire rendering block (header + content) in
ui.push_id(&contract_id)for robustness — prevents potential widget ID collisions between different contracts' +/- buttons.Testing
cargo checkandcargo fmtpassSummary by CodeRabbit
Validation
What was tested:
cargo check— verified compilation after deduplication and eguipush_idchangescargo fmt— formatting cleanResults:
ClippyCI check — pass (5m25s)Test SuiteCI check — pass (7m41s)Environment: Local macOS arm64; GitHub Actions CI (ubuntu-latest)