refactor: improve nbtc_deposit_addresses usage#328
Merged
robert-zaremba merged 4 commits intomasterfrom Jan 29, 2026
Merged
Conversation
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Contributor
Reviewer's GuideRefactors how nbtc_deposit_addresses are referenced across BTC and SUI indexer code by removing explicit pre‑lookup queries and setup-based joins, instead using inline sub-selects by deposit address and simplifying related tests, storage, and seed script queries. Entity relationship diagram for nbtc_deposit_addresses-based lookupserDiagram
nbtc_deposit_addresses {
int id PK
int setup_id FK
string deposit_address
}
setups {
int id PK
string btc_network
string sui_network
string nbtc_pkg
}
nbtc_minting {
string tx_id PK
int address_id FK
string sender
int vout
string block_hash
int block_height
string sui_recipient
int amount
string status
int retry_count
string sui_tx_id
int created_at
int updated_at
}
nbtc_utxos {
string nbtc_utxo_id PK
int address_id FK
string dwallet_id
string txid
int vout
int amount
string script_pubkey
string status
int locked_until
}
setups ||--o{ nbtc_deposit_addresses : has
nbtc_deposit_addresses ||--o{ nbtc_minting : referenced_by
nbtc_deposit_addresses ||--o{ nbtc_utxos : referenced_by
Flow diagram for inserting nbtc_minting rows using inline sub-selectsflowchart TD
A["Receive BTC or deposit transaction data"] --> B["Derive depositAddress"]
B --> C["INSERT into nbtc_minting<br/>VALUES (<br/> tx_id,<br/> (SELECT id FROM nbtc_deposit_addresses<br/> WHERE deposit_address = depositAddress),<br/> other_fields...<br/>)"]
C --> D{"Sub-select finds matching<br/>nbtc_deposit_addresses row?"}
D -- "Yes" --> E["Row inserted or updated<br/>(address_id set from sub-select)"]
D -- "No" --> F["INSERT fails due to missing<br/>matching deposit address<br/>(DB-level integrity)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several queries were simplified to look up
nbtc_deposit_addressesonly bydeposit_address(droppingsetup_id/network/nbtc_pkg filters); please confirm thatdeposit_addressis globally unique or reintroduce appropriate scoping to avoid ambiguous matches across setups/networks. - In the test helper
insertTx, the explicit pre-check that a deposit address exists was removed in favor of an inline subquery; consider restoring an explicit validation or clearer error handling so test failures are easier to diagnose when the address is missing. - In
seed-config.ts, the query was changed fromSELECT id FROM nbtc_deposit_addresses...toSELECT 1...butexecuteQuery<number>(..., "id")still appears to expect a column namedid; you may need to alias the constant (SELECT 1 AS id) or adjust the result access.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several queries were simplified to look up `nbtc_deposit_addresses` only by `deposit_address` (dropping `setup_id`/network/nbtc_pkg filters); please confirm that `deposit_address` is globally unique or reintroduce appropriate scoping to avoid ambiguous matches across setups/networks.
- In the test helper `insertTx`, the explicit pre-check that a deposit address exists was removed in favor of an inline subquery; consider restoring an explicit validation or clearer error handling so test failures are easier to diagnose when the address is missing.
- In `seed-config.ts`, the query was changed from `SELECT id FROM nbtc_deposit_addresses...` to `SELECT 1...` but `executeQuery<number>(..., "id")` still appears to expect a column named `id`; you may need to alias the constant (`SELECT 1 AS id`) or adjust the result access.
## Individual Comments
### Comment 1
<location> `packages/btcindexer/scripts/seed-config.ts:70-71` </location>
<code_context>
}
- const checkAddrQuery = `SELECT id FROM nbtc_deposit_addresses WHERE setup_id = ${setupId} AND deposit_address = '${entry.btc_address}'`;
+ const checkAddrQuery = `SELECT 1 FROM nbtc_deposit_addresses WHERE setup_id = ${setupId} AND deposit_address = '${entry.btc_address}'`;
const existingAddrId = await executeQuery<number>(checkAddrQuery, DB_NAME, local, "id");
if (existingAddrId) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Column alias in SELECT no longer matches the column name requested from executeQuery, which likely breaks the existing check.
`executeQuery<number>(..., "id")` still expects an `id` column, but the query now returns only a constant (`SELECT 1 ...`). This will likely return `undefined` or throw, so `existingAddrId` will not work as intended. Please either keep `SELECT id ...` or alias the constant as `id` (e.g. `SELECT 1 AS id ...`) to preserve the expected column name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors how nbtc_deposit_addresses are used throughout the indexers, centralizing address resolution via SQL subqueries and slightly clarifying schema intent.
Changes:
- Replace manual lookups of
nbtc_deposit_addresses.idwith inlineSELECT id FROM nbtc_deposit_addresses WHERE deposit_address = ?subqueries in both the Sui indexer and BTC indexer storage layers. - Simplify minting-related insert statements in
CFStorageand the BTC indexer test helpers to rely solely on the globally-uniquedeposit_addressinstead of joining via setups and networks. - Adjust the seeding script and the initial migration comment for
nbtc_minting.address_idto better reflect the intended relationship tonbtc_deposit_addresses.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/sui-indexer/src/storage.ts |
Simplifies UTXO insertion by resolving address_id via a subquery on deposit_address instead of a prior explicit select. |
packages/btcindexer/src/cf-storage.ts |
Refactors minting insert/UPSERT queries to derive address_id from deposit_address only, removing redundant joins on setups. |
packages/btcindexer/src/btcindexer.helpers.test.ts |
Updates test helper insertTx to match the new minting insert pattern using an inline subquery on deposit_address. |
packages/btcindexer/scripts/seed-config.ts |
Changes the existence check query for deposit addresses to select a constant rather than the id. |
packages/btcindexer/db/migrations/0001_initial_schema.sql |
Clarifies the comment on nbtc_minting.address_id to describe how it links back to nbtc_deposit_addresses.setup_id. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
70
to
72
| const checkAddrQuery = `SELECT 1 FROM nbtc_deposit_addresses WHERE setup_id = ${setupId} AND deposit_address = '${entry.btc_address}'`; | ||
| const existingAddrId = await executeQuery<number>(checkAddrQuery, DB_NAME, local, "id"); | ||
|
|
There was a problem hiding this comment.
The change from SELECT id FROM nbtc_deposit_addresses ... to SELECT 1 FROM ... means the result set no longer has an id column, but executeQuery is still called with field: "id". As a result, executeQuery will always return null for existingAddrId, so the script will try to insert the address every time and can hit the UNIQUE(deposit_address) constraint. Either keep selecting id (so the field lookup remains valid) or drop the field argument and treat a non-empty result as existence.
Contributor
Author
There was a problem hiding this comment.
should be fixed now, please recheck
Co-authored-by: sczembor <43810037+sczembor@users.noreply.github.com> Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Description
Superseeds: #326
Summary by Sourcery
Refine how nBTC deposit addresses are referenced across storage, tests, and seeding by looking them up directly via deposit_address instead of pre-validating or joining through setup metadata.
Enhancements:
Tests: