Merged
Conversation
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Contributor
Reviewer's GuideImplements sanctions checking earlier in the BTC indexing flow by screening raw transactions before mint/redeem detection, simplifies compliance API usage, and updates tests/utilities to match the new interface. Sequence diagram for early sanctions check during BTC block processingsequenceDiagram
participant Indexer
participant BlockSource
participant ComplianceService
participant Storage
loop each_finalized_block
Indexer->>BlockSource: fetchAndVerifyBlock(blockHash)
BlockSource-->>Indexer: block
loop each_tx_in_block
Indexer->>ComplianceService: isAnyBtcAddressSanctioned(senderAddresses)
alt tx_is_sanctioned
ComplianceService-->>Indexer: true
Indexer-->>Indexer: skip transaction
else tx_not_sanctioned
ComplianceService-->>Indexer: false
Indexer->>Indexer: detectMintingTx(tx, network, blockInfo)
alt is_mint_tx
Indexer->>Storage: storeDeposit(tx)
else not_mint_tx
Indexer->>Indexer: detectRedeemTx(tx, trackedRedeems)
alt is_redeem_tx
Indexer->>Storage: storeRedeem(tx)
else not_redeem_tx
Indexer-->>Indexer: continue
end
end
end
end
Indexer->>Storage: filterAlreadyMinted(finalizedTxs)
Storage-->>Indexer: txsToProcess
alt has_txs_to_process
Indexer->>Indexer: groupTransactionsByBlock(txsToProcess)
Indexer->>Indexer: prepareMintBatches(txsByBlock)
Indexer->>Indexer: executeMintBatches(batches)
else no_txs
Indexer-->>Indexer: return
end
end
Updated class diagram for Indexer sanctions and address utilitiesclassDiagram
class Indexer {
- compliance: ComplianceRpc
+ processFinalizedTransactions(): Promise~void~
+ isTxSanctioned(tx: Transaction, btcNet: Network): Promise~boolean~
+ detectMintingTx(tx: Transaction, network: Network, blockInfo: unknown): Promise~Deposit[]~
+ detectRedeemTx(tx: Transaction, trackedRedeems: string[]): boolean
- groupTransactionsByBlock(transactions: T[]): Map~string, T[]~
}
class ComplianceRpc {
<<interface>>
+ isAnyBtcAddressSanctioned(btcAddresses: string[]): Promise~boolean~
}
class BtcAddressUtils {
<<utility>>
+ extractSenderAddresses(tx: Transaction, btcNet: Network): string[]
}
class Transaction {
}
class Network {
}
class Deposit {
}
Indexer --> ComplianceRpc : uses
Indexer --> BtcAddressUtils : uses
BtcAddressUtils --> Transaction : reads
BtcAddressUtils --> Network : uses
Indexer --> Transaction : processes
Indexer --> Network : uses
Indexer --> Deposit : returns
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 2 issues, and left some high level feedback:
- In the block transaction loop,
deposits.concat();is a no-op and should either be removed or replaced withdeposits = deposits.concat(depositTxs)if concatenation was intended. - Consider marking
isTxSanctionedasprivate asyncfor consistency with how it’s used (await this.isTxSanctioned) and with other similar helper methods on the class. - In
extractSenderAddresses, the parameter namebtcNetnow represents aNetworkrather than aBtcNet; renaming it (and matching the helper’s internal naming) would make the type change clearer and reduce confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the block transaction loop, `deposits.concat();` is a no-op and should either be removed or replaced with `deposits = deposits.concat(depositTxs)` if concatenation was intended.
- Consider marking `isTxSanctioned` as `private async` for consistency with how it’s used (`await this.isTxSanctioned`) and with other similar helper methods on the class.
- In `extractSenderAddresses`, the parameter name `btcNet` now represents a `Network` rather than a `BtcNet`; renaming it (and matching the helper’s internal naming) would make the type change clearer and reduce confusion.
## Individual Comments
### Comment 1
<location> `packages/btcindexer/src/btcindexer.ts:272-274` </location>
<code_context>
+ continue;
+ }
+
+ const depositTxs = await this.detectMintingTx(tx, network, blockInfo);
+ deposits.concat();
+ if (depositTxs.length > 0) {
+ deposits.push(...depositTxs);
+ continue;
</code_context>
<issue_to_address>
**issue:** The `deposits.concat()` call is a no-op and can be removed.
Since `concat` is non-mutating and its return value isn’t used, this line is effectively dead code and can be safely removed to keep the logic clear.
</issue_to_address>
### Comment 2
<location> `packages/btcindexer/src/btcindexer.ts:265-267` </location>
<code_context>
- const txDeposits = await this.detectMintingTx(tx, network, blockInfo);
- if (txDeposits.length > 0) {
- deposits.push(...txDeposits);
+ // TODO: we should better optimise the check. In detectMintingTx and detectRedeemTx
+ // we are checking inputs and outputs, so ideally we don't repeat this.
+ if (await this.isTxSanctioned(tx, network)) {
+ logger.debug({ msg: "tx with sanctioned address", txId: tx.getId() });
+ continue;
</code_context>
<issue_to_address>
**question (bug_risk):** Sanctions are now checked before both mint and redeem detection, which changes behavior for redeems.
This now skips any tx from a sanctioned sender before both `detectMintingTx` and `detectRedeemTx`, so redeems from sanctioned addresses are also ignored. Please confirm whether the requirement is to block both mints and redeems, or only mints / different flows, and consider making that policy explicit or separating the checks if they should differ.
</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 the sanctions check mechanism in the BTC indexer to filter transactions earlier in the processing pipeline. The sanctions filtering is moved from the minting phase to the block scanning phase, preventing sanctioned transactions from being stored in the database.
Changes:
- Moved sanctions check from
processFinalizedTransactionstoscanBlockTransactionsfor earlier filtering - Replaced
filterSanctionedTxsmethod with simplerisTxSanctionedmethod that uses updated compliance API - Simplified
btc-address-utils.tsto useNetworktype directly instead ofBtcNetfor better type consistency
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/btcindexer/src/btcindexer.ts | Refactored sanctions check to filter during block scanning; removed old filterSanctionedTxs method and added new isTxSanctioned method; cleaned up code formatting |
| packages/btcindexer/src/btcindexer.test.ts | Updated test suite to test isTxSanctioned method directly with realistic test data |
| packages/btcindexer/src/btcindexer.helpers.test.ts | Updated mock compliance service to use new isAnyBtcAddressSanctioned API |
| packages/btcindexer/src/btc-address-utils.ts | Simplified function signatures to use Network type directly instead of converting from BtcNet |
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
Integrate transaction-level sanctions checks into the Bitcoin indexer and simplify the compliance interface while tightening logging and test coverage.
Bug Fixes:
Enhancements:
Tests: