feat(db): use deposit_address as PK instead of id (number) in nbtc_deposit_addresses#326
Closed
robert-zaremba wants to merge 4 commits intomasterfrom
Closed
feat(db): use deposit_address as PK instead of id (number) in nbtc_deposit_addresses#326robert-zaremba wants to merge 4 commits intomasterfrom
robert-zaremba wants to merge 4 commits intomasterfrom
Conversation
…t_addresses table Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Contributor
Reviewer's GuideRefactors the Bitcoin/nBTC indexing schema and logic so that deposit_address becomes the primary key and foreign key for nbtc_deposit_addresses, propagating this change through storage logic, queries, types, tests, and migrations to remove numeric address IDs in favor of using the deposit address string directly. ER diagram for updated nbtc_deposit_addresses keying by deposit_addresserDiagram
setups {
INTEGER id PK
TEXT btc_network
TEXT sui_network
TEXT nbtc_pkg
}
nbtc_deposit_addresses {
TEXT deposit_address PK
INTEGER setup_id FK
INTEGER is_active
}
nbtc_minting {
TEXT tx_id PK
TEXT address_id FK
TEXT sender
INTEGER vout
TEXT block_hash
INTEGER block_height
TEXT sui_recipient
INTEGER amount
TEXT status
INTEGER created_at
INTEGER updated_at
TEXT sui_tx_id
INTEGER retry_count
}
nbtc_utxos {
INTEGER nbtc_utxo_id PK
TEXT address_id FK
TEXT dwallet_id
TEXT txid
INTEGER vout
INTEGER amount
BLOB script_pubkey
TEXT status
INTEGER locked_until
}
setups ||--o{ nbtc_deposit_addresses : has
nbtc_deposit_addresses ||--o{ nbtc_minting : referenced_by
nbtc_deposit_addresses ||--o{ nbtc_utxos : referenced_by
Class diagram for updated Utxo and UtxoRow address_id typesclassDiagram
class Utxo {
+string nbtc_utxo_id
+string dwallet_id
+string txid
+number vout
+number amount
+Uint8Array script_pubkey
+string address_id
+UtxoStatus status
+number locked_until
}
class UtxoRow {
+string nbtc_utxo_id
+string dwallet_id
+string txid
+number vout
+number amount
+ArrayBuffer script_pubkey
+string address_id
+UtxoStatus status
+number locked_until
}
Utxo <--> UtxoRow
File-Level Changes
Possibly linked issues
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 left some high level feedback:
- In the initial schema, making
deposit_addressthe primary key while keeping the comment-- make sure we don't share bitcoin deposit address between setupsis slightly inconsistent with the new constraint (now it's globally unique, not per-setup); consider either changing the comment or using a composite key (e.g.(setup_id, deposit_address)) if per-setup uniqueness is actually desired. - In
CFStoragetheINSERT INTO nbtc_mintingstatements now bindaddress_iddirectly asdeposit_addressand no longer filter throughsetupsmetadata (btc_network, sui_network, nbtc_pkg); if multiple setups or networks can conceptually reuse addresses, you may want to keep an explicit join/validation to ensure the address maps to the expected setup before inserting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the initial schema, making `deposit_address` the primary key while keeping the comment `-- make sure we don't share bitcoin deposit address between setups` is slightly inconsistent with the new constraint (now it's globally unique, not per-setup); consider either changing the comment or using a composite key (e.g. `(setup_id, deposit_address)`) if per-setup uniqueness is actually desired.
- In `CFStorage` the `INSERT INTO nbtc_minting` statements now bind `address_id` directly as `deposit_address` and no longer filter through `setups` metadata (btc_network, sui_network, nbtc_pkg); if multiple setups or networks can conceptually reuse addresses, you may want to keep an explicit join/validation to ensure the address maps to the expected setup before inserting.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
Updates the D1 schema and related storage code to use the Bitcoin deposit_address (TEXT) as the primary key for nbtc_deposit_addresses, replacing the previous numeric id key.
Changes:
- Updates the initial DB schema to make
deposit_addressthe PK and changes dependentaddress_idcolumns/FKs to TEXT. - Updates Sui indexer storage/models to store and join on
deposit_addressinstead of numeric IDs. - Updates btcindexer storage code and tests to insert/query using
deposit_addressdirectly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sui-indexer/src/storage.ts | Writes/joins UTXOs using deposit_address as address_id |
| packages/sui-indexer/src/storage.test.ts | Adjusts deposit address inserts for new schema |
| packages/sui-indexer/src/models.ts | Updates Utxo.address_id type to string |
| packages/btcindexer/src/storage.test.ts | Updates test inserts to omit numeric id |
| packages/btcindexer/src/cf-storage.ts | Stores/joins minting rows using deposit_address |
| packages/btcindexer/src/btcindexer.helpers.test.ts | Updates helper insert to use deposit_address directly |
| packages/btcindexer/db/migrations/0001_initial_schema.sql | Changes PK/FKs to use deposit_address and TEXT address_id |
Comments suppressed due to low confidence (1)
packages/sui-indexer/src/storage.test.ts:529
- This test is currently effectively disabled: the calls/assertions are commented out, so
markRedeemInputVerifiedis no longer exercised by the suite. Please re-enable the test (or replace it with an updated assertion) so regressions in verification logic are still caught.
await storage.saveRedeemInputs([
{
redeem_id: 1,
utxo_id: 1,
input_index: 0,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
164
to
168
| await stmt | ||
| .bind( | ||
| u.nbtc_utxo_id, | ||
| addrRow.id, | ||
| depositAddress, | ||
| u.dwallet_id, |
There was a problem hiding this comment.
insertUtxo now writes depositAddress directly into nbtc_utxos.address_id without verifying that the derived address is registered for u.setup_id. With deposit_address being globally unique, a caller bug (wrong setup_id vs. script_pubkey) would still satisfy the FK and silently associate the UTXO with the other setup’s address. Consider restoring the invariant check by selecting setup_id from nbtc_deposit_addresses for depositAddress and throwing if it doesn’t match u.setup_id (or enforcing the relationship at the schema level).
robert-zaremba
force-pushed
the
robert/pk-nbtc-deposit-addresses
branch
from
67260bc to
8dca98b
Compare
sczembor
reviewed
Jan 28, 2026
Contributor
sczembor
left a comment
sczembor
left a comment
There was a problem hiding this comment.
i am not sure about this change. I agree that the setup id is not needed, however it makes the whole thing more clear, now we have a field address_id that its not id, but the actual address
| amount: number; | ||
| script_pubkey: Uint8Array; | ||
| address_id: number; | ||
| address_id: string; // deposit Bitcoin address |
Contributor
There was a problem hiding this comment.
i think it should be just address
Contributor
Author
There was a problem hiding this comment.
OK. I will rename to deposit_address.
Contributor
Author
|
Superseeded by: #328 |
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.
Summary by Sourcery
Switch nbtc_deposit_addresses to use deposit_address as the primary key instead of a numeric id and update related storage, queries, and schema accordingly.
Enhancements:
Tests:
Chores: