WIP Bitcoind replace lnd-onchain #438
Closed
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
jotapea
commented
Aug 10, 2021
| }) // TODO conf_target | ||
| const feerate = result.feerate | ||
| // 1 BTC/kB = 100000 satoshis/byte | ||
| fee = 100000 * feerate // TODO not sure about this: is fee by size, not a total fee |
There was a problem hiding this comment.
Confirmed that it is different from lightning's getChainFeeEstimate, which does return a total fee.
|
This is still in full WIP mode, so it contains multiple open questions as comments (which will be cleaned up after addressed). The main blocker at the moment is related to the lifecycle of the zeromq socket. I haven't found a way to be able to run multiple tests with it, at the moment it only works in a single test at a time... |
dolcalmi
reviewed
Aug 10, 2021
| throw new LoggedError(error) | ||
| } | ||
| // TODO pubkey should be known already | ||
| const { pubkey } = await this.bitcoindWalletClient.getAddressInfo({ address }) |
There was a problem hiding this comment.
I am not sure if we need this pubkey, the previous pubkey is the pubkey of lnd node that is needed to get chain data in the cron and trigger.
bitcoindSubscribers module includes logging messages test functions receiveRawBlockSubscriber and receiveRawTxSubscriber
TODO: not using SpecterWalletConstructorArgs as I am not sure the best way to add BitcoindClient type
- getBalancesDetail and getBalance bitcoind index utility functions receive BitcoindClient as parameter - replaced with OnChainMixin instance variable TODO?
- estimateSmartFee added to BitcoindClient - listTransactions added to BitcoindWalletClient - getTransaction moved to BitcoindWalletClient - getNewAddress now accepts optional address_type param - sendToAddress now accepts optional fee_rate param
- OnChainMixin methods now use bitcoind client calls instead of lnd -- getSatsAndAddressPerTx method broken up -- some TODO comments should be confirmed, is a WIP - getOnChainTransactions function no longer required - some types and comments fixed
Is the equivalent to lnd-onchain's onchainTransactionEventHandler
At the moment is only working for a single instance, which is not compatible with the current test's idempotence requirement
- "create hot wallet" test added - TODO "funds lnd1 node" updated (confirm use of bankowner)
Should be done with a mock ideally
These wallet actions add complexity to the integration tests, so will be addressed later
This reverts commit 22cbf98.
without the removal of tests
which is commented out for tests to pass if this is the only bitcoind zmq/subscriber instance...
jotapea
force-pushed
the
bitcoind-replace-lndonchain
branch
from
82618e1
to
cdabe8d
Compare
only one test at a time works
|
@jotapea you should have the PR as "draft" when they are work in progress |
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.
New branch, but still a work in progress, for the original #317.