Improve find wallet for redemption logic #651
Conversation
Here we expose a function allowing to get the transaction history for given Bitcoin address.
Here we expose a function allowing to determine the wallet main UTXO based on the wallet recent transactions history and the main UTXO hash stored on the Bridge contract.
The `findWalletForRedemption` function should rely on `determineWalletMainUtxo` wallet while seeking for current wallet main UTXO. Otherwise, it may not find the main UTXO in case there is a wallet state drift between Bitcoin and Ethereum chains.
There was a problem hiding this comment.
The code looks good to me. I added some minor comments.
Leaving it in @r-czajkowski's hands.
| * a specific number of last transaction. For example, limit = 5 will | ||
| * return only the last 5 transactions for the given address. | ||
| */ | ||
| getTransactionHistory(address: string, limit?: number): Promise<Transaction[]> |
There was a problem hiding this comment.
Idea for the future: we could add a parameter specifying the required number of confirmations.
The "confirmed transaction" is a fuzzy term. In this context, it's a transaction with 1 confirmation but in some other contexts, we do not consider the transaction as confirmed if it has less than 6 confirmations.
| historyItems.length > limit | ||
| ) { | ||
| historyItems = historyItems.slice(-limit) | ||
| } |
There was a problem hiding this comment.
I was expecting blockchain_scripthash_getHistory to accept limit as a parameter but yeah, I see in the linked docs it is not possible. 😞
| // eslint-disable-next-line camelcase | ||
| type HistoryItem = { height: number; tx_hash: string } | ||
|
|
||
| let historyItems: HistoryItem[] = await this.withBackoffRetrier< |
There was a problem hiding this comment.
Nitpicking: Instead of having await, I think we could use .then to chain promises together.
There was a problem hiding this comment.
I think async/await style leads to more readable code and is generally preferred over then. It is also widely used in our codebase and I think mixing both styles is not necessarily the best idea.
| @@ -108,6 +108,28 @@ describe("Electrum", () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe("getTransactionHistory", () => { | |||
| it("should return proper transaction history for the given address", async () => { | |||
| // https://live.blockcypher.com/btc-testnet/address/tb1qumuaw3exkxdhtut0u85latkqfz4ylgwstkdzsx | |||
There was a problem hiding this comment.
Just confirming: this is an address controlled by us and it will not execute any new transactions in the future? 😆
Refs: threshold-network/token-dashboard#553
Here we improve the logic of
findWalletForRedemptionfunction to handle the corner case when a wallet's main UTXO known to theBridgewas actually spent on Bitcoin but that change was not yet reflected to theBridge. So far,findWalletForRedemptionrelied onfindAllUnspentTransactionOutputswhich does not return spent UTXOs so determining the actual main UTXO was not possible in the aforementioned corner case. Now, thefindWalletForRedemptionlooks for recent wallet transaction history on Bitcoin and tries to match the actual main UTXO based on that. This change will allow submitting new redemption requests without downtime, even if the target wallet is performing an action at the moment.