Replace bcoin with bitcoinjs-lib for deposits #702
Merged
Conversation
This file contains 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
tomaszslabon
requested review from
michalsmiarowski and
lukasz-zimnoch
and removed request for
michalsmiarowski
tomaszslabon
changed the base branch from
main
to
deposit-sweep-use-bitcoinjs-lib
tomaszslabon
force-pushed
the
deposit-use-bitcoinjs-lib
branch
from
ef82753
to
ec8a499
Compare
Comment on lines
+239
to
+240
| // TODO: Add support for other utxo types along with unit tests for the | ||
| // given type. |
There was a problem hiding this comment.
Do we plan to do this work as part of #695 or separately? If this is something quick I'd vote for the first option. Otherwise, we can postpone it now and return here after the release of the reworked API. On a high level, our work plan is remove bcoin -> bump node -> release v1.4.0 -> API changes -> release v2.0.0 -> improvements so we can return here during improvements phase. WDYT?
There was a problem hiding this comment.
Let's postpone it, as supporting more types require adding unit tests.
tomaszslabon
force-pushed
the
deposit-use-bitcoinjs-lib
branch
from
ac2f64d
to
36ad06f
Compare
lukasz-zimnoch
approved these changes
Oct 3, 2023
michalsmiarowski
added a commit
to threshold-network/token-dashboard
that referenced
this pull request
Oct 4, 2023
There was a change in keep-network/tbtc-v2#702 in which we've added additional arguments to `assembleDepositTransaction` function. The order of the arguments was also changed. Because of that we couldn't build the project because we use that function in our dApp mock of the bitcoin client. We are fixing this by passing two additional arguments to the `assembleDepositTransaction` function - `network` and `fee`. We are also making sure that we pass the arguments in the correct order.
lukasz-zimnoch
added a commit
that referenced
this pull request
Oct 10, 2023
Here we pull changes from #702
lukasz-zimnoch
added a commit
that referenced
this pull request
Oct 10, 2023
Here we pull changes from #702
theref
pushed a commit
to theref/token-dashboard
that referenced
this pull request
Oct 11, 2023
There was a change in keep-network/tbtc-v2#702 in which we've added additional arguments to `assembleDepositTransaction` function. The order of the arguments was also changed. Because of that we couldn't build the project because we use that function in our dApp mock of the bitcoin client. We are fixing this by passing two additional arguments to the `assembleDepositTransaction` function - `network` and `fee`. We are also making sure that we pass the arguments in the correct order.
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.
Refs: #695.
This PR replaces the
bcoinlibrary withbitcoinjs-libfor deposits. The functions generally works in the same way as before the change.One difference is that UTXOs for funding the transaction and the value of fee are passed into the function as arguments.
In the future, we could consider using coinselect for calculating fee and selecting UTXOs.
Another difference is that only one type of funding UTXO is allowed (P2WPKH). Other types should be added along with unit tests.