-
Notifications
You must be signed in to change notification settings - Fork 782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REF: move segwit bech32 transaction class to TS #6236
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs work
@@ -38,11 +42,11 @@ export class HDSegwitBech32Transaction { | |||
* @returns {Promise<void>} | |||
* @private | |||
*/ | |||
async _fetchTxhexAndDecode() { | |||
async _fetchTxhexAndDecode(): Promise<bitcoin.Transaction> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should be Promise<void>
const hexes = await BlueElectrum.multiGetTransactionByTxid([this._txid], 10, false); | ||
this._txhex = hexes[this._txid]; | ||
if (!this._txhex) throw new Error("Transaction can't be found in mempool"); | ||
this._txDecoded = bitcoin.Transaction.fromHex(this._txhex); | ||
return bitcoin.Transaction.fromHex(this._txhex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it was designed to not return a transaction, but place decoded tx in this._txDecoded
.
with this change I suppose some tests will fail
async getMaxUsedSequence() { | ||
if (!this._txDecoded) await this._fetchTxhexAndDecode(); | ||
async getMaxUsedSequence(): Promise<number> { | ||
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of that change in _fetchTxhexAndDecode()
you now have to rewrite all that checks in different methods, bloating the diff for no good reason
async _fetchRemoteTx() { | ||
const result = await BlueElectrum.multiGetTransactionByTxid([this._txid || this._txDecoded.getId()]); | ||
this._remoteTx = Object.values(result)[0]; | ||
async _fetchRemoteTx(): Promise<ElectrumTransaction> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same story
@@ -91,7 +95,7 @@ export class HDSegwitBech32Transaction { | |||
*/ | |||
async getRemoteConfirmationsNum() { | |||
if (!this._remoteTx) await this._fetchRemoteTx(); | |||
return this._remoteTx.confirmations || 0; // stupid undefined | |||
return this._remoteTx?.confirmations || 0; // stupid undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe change to ??
operator?
if (!this._wallet) throw new Error('Wallet required for this method'); | ||
if (!this._remoteTx) await this._fetchRemoteTx(); | ||
|
||
const { feeRate, targets, changeAmount, utxos } = await this.getInfo(); | ||
|
||
const newTargets: { value?: number; address: string }[] = targets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe if getInfo()
was properly typed this newTargets
wouldnt even be needed
// not checking emptiness on purpose: it could unpredictably generate too far address because of unconfirmed tx. | ||
} | ||
|
||
return this._wallet.createTransaction(utxos, targets, newFeerate, myAddress, (await this.getMaxUsedSequence()) + 1); | ||
return this._wallet.createTransaction(utxos, targets, newFeerate, myAddress, (await this.getMaxUsedSequence()) + 1, false, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you introduced new newTargets
and worked with it, but pass to createTransaction
old targets
if (!this._remoteTx) this._remoteTx = await this._fetchRemoteTx(); | ||
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same story
|
||
return { tx, inputs, outputs, fee }; | ||
if (add > 128) return { tx, fee }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats this 128? is it a leftover from debug?
const combinedFeeRate = (oldFee + fee) / (this._txDecoded.virtualSize() + tx.virtualSize()); // avg | ||
if (Math.round(combinedFeeRate) < newFeerate) { | ||
add *= 2; | ||
if (!add) add = 2; | ||
} else { | ||
// reached target feerate | ||
break; | ||
return { tx, fee }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd prefer break
so there is only a single return statement in the function
No description provided.