Skip to content
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.

Sign up for GitHub

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

Jump to bottom

REF: move segwit bech32 transaction class to TS #6236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

REF: move segwit bech32 transaction class to TS #6236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1316b13
REF: move segwit bech32 transaction class to TS
abhiShandy Mar 6, 2024
96ab4a7
Merge branch 'master' into ref/wallet2-ts
abhiShandy Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
102 changes: 56 additions & 46 deletions class/hd-segwit-bech32-transaction.js → class/hd-segwit-bech32-transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { HDSegwitBech32Wallet } from './wallets/hd-segwit-bech32-wallet';
import { SegwitBech32Wallet } from './wallets/segwit-bech32-wallet';
const bitcoin = require('bitcoinjs-lib');
const BlueElectrum = require('../blue_modules/BlueElectrum');
const BigNumber = require('bignumber.js');
import bitcoin from 'bitcoinjs-lib';
import BlueElectrum, { type ElectrumTransaction } from '../blue_modules/BlueElectrum';
import BigNumber from 'bignumber.js';

/**
* Represents transaction of a BIP84 wallet.
Expand All @@ -14,14 +14,18 @@ export class HDSegwitBech32Transaction {
* @param txid {string|null} If txhex not present - txid whould be present
* @param wallet {HDSegwitBech32Wallet|null} If set - a wallet object to which transacton belongs
*/
constructor(txhex, txid, wallet) {
if (!txhex && !txid) throw new Error('Bad arguments');
_txhex: string;
_txid: string;
_wallet?: HDSegwitBech32Wallet;
_txDecoded?: bitcoin.Transaction;
_remoteTx: ElectrumTransaction | null;
constructor(txhex: string | null, txid: string | null, wallet: HDSegwitBech32Wallet | null) {
if (!txhex || !txid) throw new Error('Bad arguments');
this._txhex = txhex;
this._txid = txid;

if (wallet) {
if (wallet.type === HDSegwitBech32Wallet.type) {
/** @type {HDSegwitBech32Wallet} */
this._wallet = wallet;
} else {
throw new Error('Only HD Bech32 wallets supported');
Expand All @@ -38,11 +42,11 @@ export class HDSegwitBech32Transaction {
* @returns {Promise<void>}
* @private
*/
async _fetchTxhexAndDecode() {
async _fetchTxhexAndDecode(): Promise<bitcoin.Transaction> {
Copy link
Member

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);
Copy link
Member

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

}

/**
Expand All @@ -51,8 +55,8 @@ export class HDSegwitBech32Transaction {
*
* @returns {Promise<number>}
*/
async getMaxUsedSequence() {
if (!this._txDecoded) await this._fetchTxhexAndDecode();
async getMaxUsedSequence(): Promise<number> {
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode();
Copy link
Member

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


let max = 0;
for (const inp of this._txDecoded.ins) {
Expand All @@ -67,7 +71,7 @@ export class HDSegwitBech32Transaction {
*
* @returns {Promise<boolean>}
*/
async isSequenceReplaceable() {
async isSequenceReplaceable(): Promise<boolean> {
return (await this.getMaxUsedSequence()) < bitcoin.Transaction.DEFAULT_SEQUENCE;
}

Expand All @@ -79,9 +83,9 @@ export class HDSegwitBech32Transaction {
* @returns {Promise<void>}
* @private
*/
async _fetchRemoteTx() {
const result = await BlueElectrum.multiGetTransactionByTxid([this._txid || this._txDecoded.getId()]);
this._remoteTx = Object.values(result)[0];
async _fetchRemoteTx(): Promise<ElectrumTransaction> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same story

const result = await BlueElectrum.multiGetTransactionByTxid([this._txid]);
return Object.values(result)[0];
}

/**
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change to ?? operator?

}

/**
Expand All @@ -101,13 +105,13 @@ export class HDSegwitBech32Transaction {
*
* @returns {Promise<boolean>}
*/
async isOurTransaction() {
async isOurTransaction(): Promise<boolean> {
if (!this._wallet) throw new Error('Wallet required for this method');
let found = false;
for (const tx of this._wallet.getTransactions()) {
if (tx.txid === (this._txid || this._txDecoded.getId())) {
if (tx.txid === (this._txid || this._txDecoded?.getId())) {
// its our transaction, and its spending transaction, which means we initiated it
if (tx.value < 0) found = true;
if (tx.value && tx.value < 0) found = true;
}
}
return found;
Expand All @@ -124,8 +128,8 @@ export class HDSegwitBech32Transaction {
if (!this._wallet) throw new Error('Wallet required for this method');
let found = false;
for (const tx of this._wallet.getTransactions()) {
if (tx.txid === (this._txid || this._txDecoded.getId())) {
if (tx.value > 0) found = true;
if (tx.txid === (this._txid || this._txDecoded?.getId())) {
if (tx.value && tx.value > 0) found = true;
}
}
return found;
Expand All @@ -144,14 +148,12 @@ export class HDSegwitBech32Transaction {
*/
async getInfo() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldve added return type here as well

if (!this._wallet) throw new Error('Wallet required for this method');
if (!this._remoteTx) await this._fetchRemoteTx();
if (!this._txDecoded) await this._fetchTxhexAndDecode();
if (!this._remoteTx) this._remoteTx = await this._fetchRemoteTx();
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode();
Comment on lines +151 to +152
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same story


const prevInputs = [];
for (const inp of this._txDecoded.ins) {
let reversedHash = Buffer.from(inp.hash).reverse();
reversedHash = reversedHash.toString('hex');
prevInputs.push(reversedHash);
prevInputs.push(Buffer.from(inp.hash).reverse().toString('hex'));
}

const prevTransactions = await BlueElectrum.multiGetTransactionByTxid(prevInputs);
Expand All @@ -160,13 +162,12 @@ export class HDSegwitBech32Transaction {
let wentIn = 0;
const utxos = [];
for (const inp of this._txDecoded.ins) {
let reversedHash = Buffer.from(inp.hash).reverse();
reversedHash = reversedHash.toString('hex');
const reversedHash = Buffer.from(inp.hash).reverse().toString('hex');
if (prevTransactions[reversedHash] && prevTransactions[reversedHash].vout && prevTransactions[reversedHash].vout[inp.index]) {
let value = prevTransactions[reversedHash].vout[inp.index].value;
value = new BigNumber(value).multipliedBy(100000000).toNumber();
wentIn += value;
const address = SegwitBech32Wallet.witnessToAddress(inp.witness[inp.witness.length - 1]);
const address = SegwitBech32Wallet.witnessToAddress(inp.witness[inp.witness.length - 1].toString('hex'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird change. i just looked at this method, and it shoulsd return either string or false.
we also have tests for that: tests/unit/hd-segwit-p2sh-wallet.test.js

where this toString('hex')) comes from..?

utxos.push({ vout: inp.index, value, txId: reversedHash, address });
}
}
Expand Down Expand Up @@ -205,7 +206,7 @@ export class HDSegwitBech32Transaction {
unconfirmedUtxos.push({
vout: outp.n,
value,
txId: this._txid || this._txDecoded.getId(),
txId: this._txid || this._txDecoded?.getId(),
address,
});
}
Expand All @@ -223,7 +224,7 @@ export class HDSegwitBech32Transaction {
*/
async thereAreUnknownInputsInTx() {
if (!this._wallet) throw new Error('Wallet required for this method');
if (!this._txDecoded) await this._fetchTxhexAndDecode();
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same story


const spentUtxos = this._wallet.getDerivedUtxoFromOurTransaction(true);
for (const inp of this._txDecoded.ins) {
Expand All @@ -248,13 +249,14 @@ export class HDSegwitBech32Transaction {
*/
async canCancelTx() {
if (!this._wallet) throw new Error('Wallet required for this method');
if (!this._txDecoded) await this._fetchTxhexAndDecode();
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same story


if (await this.thereAreUnknownInputsInTx()) return false;

// if theres at least one output we dont own - we can cancel this transaction!
for (const outp of this._txDecoded.outs) {
if (!this._wallet.weOwnAddress(SegwitBech32Wallet.scriptPubKeyToAddress(outp.script))) return true;
const address = SegwitBech32Wallet.scriptPubKeyToAddress(outp.script.toString());
if (address && !this._wallet.weOwnAddress(address)) return true;
}

return false;
Expand All @@ -277,7 +279,7 @@ export class HDSegwitBech32Transaction {
* @param newFeerate {number} Sat/byte. Should be greater than previous tx feerate
* @returns {Promise<{outputs: Array, tx: Transaction, inputs: Array, fee: Number}>}
*/
async createRBFcancelTx(newFeerate) {
async createRBFcancelTx(newFeerate: number) {
if (!this._wallet) throw new Error('Wallet required for this method');
if (!this._remoteTx) await this._fetchRemoteTx();

Expand All @@ -292,6 +294,8 @@ export class HDSegwitBech32Transaction {
newFeerate,
/* meaningless in this context */ myAddress,
(await this.getMaxUsedSequence()) + 1,
false,
0,
);
}

Expand All @@ -302,39 +306,41 @@ export class HDSegwitBech32Transaction {
* @param newFeerate {number} Sat/byte
* @returns {Promise<{outputs: Array, tx: Transaction, inputs: Array, fee: Number}>}
*/
async createRBFbumpFee(newFeerate) {
async createRBFbumpFee(newFeerate: number) {
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;
Copy link
Member

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


if (newFeerate <= feeRate) throw new Error('New feerate should be bigger than the old one');
const myAddress = await this._wallet.getChangeAddressAsync();

if (changeAmount === 0) delete targets[0].value;
if (changeAmount === 0) delete newTargets[0].value;
// looks like this was sendMAX transaction (because there was no change), so we cant reuse amount in this
// target since fee wont change. removing the amount so `createTransaction` will sendMAX correctly with new feeRate

if (targets.length === 0) {
// looks like this was cancelled tx with single change output, so it wasnt included in `this.getInfo()` targets
// so we add output paying ourselves:
targets.push({ address: this._wallet._getInternalAddressByIndex(this._wallet.next_free_change_address_index) });
newTargets.push({ address: this._wallet._getInternalAddressByIndex(this._wallet.next_free_change_address_index) });
// 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);
Copy link
Member

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

}

/**
* Creates a CPFP transaction that can bumps fee of previous one (spends created but not confirmed outputs
* that belong to us). Note, this cannot add more utxo in CPFP transaction if newFeerate is too high
*
* @param newFeerate {number} sat/byte
* @returns {Promise<{outputs: Array, tx: Transaction, inputs: Array, fee: Number}>}
*/
async createCPFPbumpFee(newFeerate) {
async createCPFPbumpFee(newFeerate: number): Promise<{ tx: bitcoin.Transaction; fee: number }> {
if (!this._wallet) throw new Error('Wallet required for this method');
if (!this._remoteTx) await this._fetchRemoteTx();
if (!this._remoteTx) this._remoteTx = await this._fetchRemoteTx();
if (!this._txDecoded) this._txDecoded = await this._fetchTxhexAndDecode();
Comment on lines +342 to +343
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same story


const { feeRate, fee: oldFee, unconfirmedUtxos } = await this.getInfo();

Expand All @@ -346,25 +352,29 @@ export class HDSegwitBech32Transaction {
const targetFeeRate = 2 * newFeerate - feeRate;

let add = 0;
while (add <= 128) {
// eslint-disable-next-line no-var
var { tx, inputs, outputs, fee } = this._wallet.createTransaction(
while (true) {
const { tx, fee } = this._wallet.createTransaction(
unconfirmedUtxos,
[{ address: myAddress }],
targetFeeRate + add,
myAddress,
HDSegwitBech32Wallet.defaultRBFSequence,
false,
0,
);

if (!tx) throw new Error('Failed to create CPFP transaction');

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 };
Copy link
Member

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

}
}

return { tx, inputs, outputs, fee };
if (add > 128) return { tx, fee };
Copy link
Member

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?

}
}
}
4 changes: 2 additions & 2 deletions class/wallets/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ export type Utxo = {
*/
export type CreateTransactionUtxo = {
txId: string;
txid: string; // TODO: same as txId, do we really need it?
txhex: string;
txid?: string; // TODO: same as txId, do we really need it?
txhex?: string;
vout: number;
value: number;
script?: {
Expand Down