diff --git a/modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts b/modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts index 95e3658680..69e3601271 100644 --- a/modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts +++ b/modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts @@ -7,6 +7,7 @@ import { pvmSerial, Credential, TransferOutput, + TransferableOutput, } from '@flarenetwork/flarejs'; import { BuildTransactionError, NotSupported, TransactionType } from '@bitgo/sdk-core'; import { BaseCoin as CoinConfig } from '@bitgo/statics'; @@ -210,12 +211,10 @@ export class PermissionlessDelegatorTxBuilder extends TransactionBuilder { } /** - * Get the user's address (index 0) for delegation. + * Get the user's address (index 0) for default reward address. * - * For delegation transactions, we use only the user key because: - * 1. On-chain rewards go to the C-chain address derived from the delegator's public key - * 2. Using the user key ensures rewards go to the user's corresponding C-chain address - * 3. The user key is at index 0 in the fromAddresses array (BitGo convention: [user, bitgo, backup]) + * The user key is at index 0 in the fromAddresses array (BitGo convention: [user, bitgo, backup]). + * This is used as the default rewardAddress parameter (though the parameter is ignored by protocol). * * @returns Buffer containing the user's address * @protected @@ -237,8 +236,9 @@ export class PermissionlessDelegatorTxBuilder extends TransactionBuilder { * Uses pvm.e.newAddPermissionlessDelegatorTx (post-Etna API). * * Note: The rewardAddresses parameter is accepted by the API but does NOT affect - * where rewards are sent on-chain - rewards always go to the C-chain address - * derived from the delegator's public key (user key at index 0). + * where rewards are sent on-chain. Rewards accrue to C-chain addresses derived + * from the P-chain addresses in the stake outputs. The stake outputs contain the + * addresses from fromAddressesBytes (sorted to match UTXO owner order). * @protected */ protected buildFlareTransaction(): void { @@ -275,19 +275,24 @@ export class PermissionlessDelegatorTxBuilder extends TransactionBuilder { } const utxos = utils.decodedToUtxos(this.transaction._utxos, this.transaction._network.assetId); - // Use only the user key (index 0) for fromAddressesBytes - // This ensures the C-chain reward address is derived from the user's public key + // Get user address for default reward address derivation const userAddress = this.getUserAddress(); const rewardAddresses = this.transaction._rewardAddresses.length > 0 ? this.transaction._rewardAddresses : [userAddress]; // Use Etna (post-fork) API - pvm.e.newAddPermissionlessDelegatorTx + // IMPORTANT: Sort fromAddresses to match the sorted order in UTXOs + // The SDK sorts UTXO addresses (utils.ts:574) before passing to FlareJS, + // so fromAddressesBytes must also be sorted to match UTXO owner addresses + const fromAddressBuffers = this.transaction._fromAddresses.map((addr) => Buffer.from(addr)); + const sortedFromAddresses = utils.sortAddressBuffersByHex(fromAddressBuffers); + const delegatorTx = pvm.e.newAddPermissionlessDelegatorTx( { end: this._endTime, feeState: this._feeState, - fromAddressesBytes: [userAddress], + fromAddressesBytes: sortedFromAddresses, nodeId: this._nodeID, rewardAddresses: rewardAddresses, start: this._startTime, @@ -298,7 +303,66 @@ export class PermissionlessDelegatorTxBuilder extends TransactionBuilder { this.transaction._context ); - this.transaction.setTransaction(delegatorTx as UnsignedTx); + // Fix change output threshold bug (same as ExportInPTxBuilder) + const flareUnsignedTx = delegatorTx as UnsignedTx; + const innerTx = flareUnsignedTx.getTx() as pvmSerial.AddPermissionlessDelegatorTx; + const changeOutputs = innerTx.baseTx.outputs; + let correctedDelegatorTx: pvmSerial.AddPermissionlessDelegatorTx = innerTx; + + if (changeOutputs.length > 0 && this.transaction._threshold > 1) { + // Only apply fix for multisig wallets (threshold > 1) + const allWalletAddresses = this.transaction._fromAddresses.map((addr) => Buffer.from(addr)); + + const correctedChangeOutputs = changeOutputs.map((output) => { + const transferOut = output.output as TransferOutput; + + const assetIdStr = utils.flareIdString(Buffer.from(output.assetId.toBytes()).toString('hex')).toString(); + return TransferableOutput.fromNative( + assetIdStr, + transferOut.amount(), + allWalletAddresses, + this.transaction._locktime, + this.transaction._threshold // Fix: use wallet's threshold instead of FlareJS's default (1) + ); + }); + + correctedDelegatorTx = this.createCorrectedDelegatorTx(innerTx, correctedChangeOutputs); + } + + // Create new UnsignedTx with corrected change outputs + const fixedUnsignedTx = new UnsignedTx(correctedDelegatorTx, [], new FlareUtils.AddressMaps([]), []); + + this.transaction.setTransaction(fixedUnsignedTx); + } + + /** + * Create a corrected AddPermissionlessDelegatorTx with the given change outputs. + * This is necessary because FlareJS's newAddPermissionlessDelegatorTx doesn't support setting + * the threshold and locktime for change outputs - it defaults to threshold=1. + * + * FlareJS declares baseTx.outputs as readonly, so we use Object.defineProperty + * to override the property with the corrected outputs. This is a workaround until + * FlareJS adds proper support for change output thresholds. + * + * @param originalTx - The original AddPermissionlessDelegatorTx + * @param correctedOutputs - The corrected change outputs with proper threshold + * @returns A new AddPermissionlessDelegatorTx with the corrected change outputs + */ + private createCorrectedDelegatorTx( + originalTx: pvmSerial.AddPermissionlessDelegatorTx, + correctedOutputs: TransferableOutput[] + ): pvmSerial.AddPermissionlessDelegatorTx { + // FlareJS declares baseTx.outputs as `public readonly outputs: readonly TransferableOutput[]` + // We use Object.defineProperty to override the readonly property with our corrected outputs. + // This is necessary because FlareJS's newAddPermissionlessDelegatorTx doesn't support change output threshold/locktime. + Object.defineProperty(originalTx.baseTx, 'outputs', { + value: correctedOutputs, + writable: false, + enumerable: true, + configurable: true, + }); + + return originalTx; } /** diff --git a/modules/sdk-coin-flrp/test/unit/lib/permissionlessDelegatorTxBuilder.ts b/modules/sdk-coin-flrp/test/unit/lib/permissionlessDelegatorTxBuilder.ts index c5a853935b..a1a77e5b08 100644 --- a/modules/sdk-coin-flrp/test/unit/lib/permissionlessDelegatorTxBuilder.ts +++ b/modules/sdk-coin-flrp/test/unit/lib/permissionlessDelegatorTxBuilder.ts @@ -3,7 +3,7 @@ import 'should'; import { coins } from '@bitgo/statics'; import { TransactionType } from '@bitgo/sdk-core'; import { TransactionBuilderFactory, PermissionlessDelegatorTxBuilder, Transaction } from '../../../src/lib'; -import { SEED_ACCOUNT, ACCOUNT_1, CONTEXT } from '../../resources/account'; +import { SEED_ACCOUNT, ACCOUNT_1, ACCOUNT_3, CONTEXT } from '../../resources/account'; import utils from '../../../src/lib/utils'; import { DELEGATION_TX_2 } from '../../resources/transactionData/delegatorTx'; @@ -284,4 +284,370 @@ describe('Flrp PermissionlessDelegatorTxBuilder', () => { tx.outputs.length.should.be.above(0); }); }); + + describe('build with UTXOs (comprehensive tests)', () => { + const mockFeeState = { capacity: 100000n, excess: 0n, price: 1000n, timestamp: '1234567890' }; + const MIN_DELEGATION_DURATION = 14 * 24 * 60 * 60; // 14 days (1209600 seconds) + const MIN_DELEGATION_AMOUNT = BigInt(50000 * 1e9); // 50000 FLR in nanoFLR + + function createTestUtxos(addresses: string[], threshold: number, amount = '60000000000000') { + return [ + { + outputID: 7, + txid: '2NWd9hrSGkWJWyTu4DnSM1qQSUT2DVm8uqwFk4wQRFLAmcsHQz', + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount, + threshold, + addresses, + locktime: '0', + }, + ]; + } + + it('should build delegation transaction with threshold=2 and 3 addresses (BitGo 2-of-3 multisig)', async () => { + const builder = factory.getDelegatorBuilder(); + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet, ACCOUNT_3.address]; + const utxos = createTestUtxos(addresses, 2); + + const tx = await builder + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + tx.should.be.instanceof(Transaction); + tx.type.should.equal(TransactionType.AddPermissionlessDelegator); + }); + + it('should build delegation transaction with threshold=2 and 2 addresses', async () => { + const builder = factory.getDelegatorBuilder(); + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet]; + const utxos = createTestUtxos(addresses, 2); + + const tx = await builder + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + tx.should.be.instanceof(Transaction); + tx.type.should.equal(TransactionType.AddPermissionlessDelegator); + }); + + it('should fail when UTXO amount is insufficient', async () => { + const builder = factory.getDelegatorBuilder(); + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet]; + const utxos = createTestUtxos(addresses, 2, '1000000000000'); + + await builder + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build() + .should.be.rejected(); + }); + + it('should build delegation with multiple UTXOs', async () => { + const builder = factory.getDelegatorBuilder(); + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet, ACCOUNT_3.address]; + // Create multiple UTXOs with different txids + const utxos = [ + { + outputID: 7, + txid: '2NWd9hrSGkWJWyTu4DnSM1qQSUT2DVm8uqwFk4wQRFLAmcsHQz', + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '30000000000000', // 30k FLR + threshold: 2, + addresses, + locktime: '0', + }, + { + outputID: 7, + txid: '2kSSHWKZH7uJ1FfSpgRaPfgPpnT4915QVXPAMw6HjfFQVJ1QFx', + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '25000000000000', // 25k FLR + threshold: 2, + addresses, + locktime: '0', + }, + { + outputID: 7, + txid: '2E6Xuqf3i6TnwH6zjt4K6jNDR2ooj1DTJpTFZ6SgZZkRJ4ADSe', + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '10000000000000', // 10k FLR + threshold: 2, + addresses, + locktime: '0', + }, + ]; + // Total: 65k FLR > 50k minimum + + const tx = await builder + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + tx.should.be.instanceof(Transaction); + tx.type.should.equal(TransactionType.AddPermissionlessDelegator); + // FlareJS optimizes to use minimum UTXOs needed, may not consume all 3 + tx.inputs.length.should.be.greaterThan(0); + }); + + /** + * Verifies fix for FlareJS Bug - Change output threshold + * + * FlareJS's pvm.e.newAddPermissionlessDelegatorTx() defaults change outputs to threshold=1. + * We implement a fix (similar to ExportInPTxBuilder) to correct change outputs to threshold=2 + * for multisig wallets, maintaining security. + */ + it('should create change output with threshold=2 for multisig wallet (bug fixed)', async () => { + const builder = factory.getDelegatorBuilder(); + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet, ACCOUNT_3.address]; + const utxos = createTestUtxos(addresses, 2, '60000000000000'); // 60k FLR + + const tx = await builder + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) // Stake 50k + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + // Verify change exists using explainTransaction + const explanation = tx.explainTransaction(); + const changeAmount = BigInt(explanation.changeAmount); + Number(changeAmount).should.be.greaterThan( + Number(BigInt('9000000000000')), + 'Should have change (at least 9k after fees)' + ); + + explanation.changeOutputs.length.should.be.greaterThan(0, 'Should have change outputs'); + + // Verify the fix: change outputs should now have threshold=2 + const flareTransaction = (tx as any)._flareTransaction as any; + const innerTx = flareTransaction.getTx(); + const changeOutputs = innerTx.baseTx.outputs; + + changeOutputs.forEach((output: any) => { + const transferOut = output.output; + const threshold = transferOut.outputOwners.threshold.value(); + threshold.should.equal(2, 'Change output should have threshold=2 (bug fixed!)'); + }); + }); + + it('should have change output addresses matching wallet addresses', async () => { + const builder = factory.getDelegatorBuilder(); + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet, ACCOUNT_3.address]; + const utxos = createTestUtxos(addresses, 2, '60000000000000'); + + const tx = await builder + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + // Verify change exists and addresses match using explainTransaction + const explanation = tx.explainTransaction(); + const changeAmount = BigInt(explanation.changeAmount); + Number(changeAmount).should.be.greaterThan(0, 'Should have change output'); + + explanation.changeOutputs.length.should.be.greaterThan(0); + + // Verify change output addresses match wallet addresses (sorted) + const sortedFromAddresses = (tx as Transaction).fromAddresses.slice().sort().join('~'); + + explanation.changeOutputs.forEach((output) => { + output.address.should.equal(sortedFromAddresses, 'Change output addresses should match wallet addresses'); + }); + }); + }); + + describe('multiple delegations to same validator', () => { + const mockFeeState = { capacity: 100000n, excess: 0n, price: 1000n, timestamp: '1234567890' }; + const MIN_DELEGATION_DURATION = 14 * 24 * 60 * 60; + const MIN_DELEGATION_AMOUNT = BigInt(50000 * 1e9); + + it('should support creating multiple delegations to same validator with different time periods', async () => { + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet, ACCOUNT_3.address]; + + // First delegation: Jan 1 - Mar 1 + const builder1 = factory.getDelegatorBuilder(); + const utxos1 = [ + { + outputID: 7, + txid: '2NWd9hrSGkWJWyTu4DnSM1qQSUT2DVm8uqwFk4wQRFLAmcsHQz', + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '100000000000000', // 100k FLR + threshold: 2, + addresses, + locktime: '0', + }, + ]; + + const tx1 = await builder1 + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(now + 60 + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) // 50k + .fromPubKey(addresses) + .decodedUtxos(utxos1) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + tx1.should.be.instanceof(Transaction); + tx1.type.should.equal(TransactionType.AddPermissionlessDelegator); + + // Second delegation: Feb 1 - Apr 1 (overlapping period, different start/end) + // Simulating using a different UTXO (in practice, could be change from first delegation) + const builder2 = factory.getDelegatorBuilder(); + const utxos2 = [ + { + outputID: 7, + txid: '2QttuE1MNRPEvLPdhB8t6WpC91VKfvigHvf1PKrNQ6BGgB6hZw', // Valid CB58 txid + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '50010000000000', // 50010 FLR (50k stake + extra for fees) + threshold: 2, + addresses, + locktime: '0', + }, + ]; + + const tx2 = await builder2 + .nodeID(validNodeID) // Same validator! + .startTime(now + 60 + 30 * 24 * 60 * 60) // 30 days later + .endTime(now + 60 + 30 * 24 * 60 * 60 + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) // Another 50k + .fromPubKey(addresses) + .decodedUtxos(utxos2) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + tx2.should.be.instanceof(Transaction); + tx2.type.should.equal(TransactionType.AddPermissionlessDelegator); + + // Both transactions should be valid and independent + tx1.id.should.not.equal(tx2.id); + }); + + it('should support sequential delegations (one ends, next begins)', async () => { + const now = Math.floor(Date.now() / 1000); + const validNodeID = 'NodeID-AK7sPBsZM9rQwse23aLhEEBPHZD5gkLrL'; + const addresses = [SEED_ACCOUNT.addressTestnet, ACCOUNT_1.addressTestnet, ACCOUNT_3.address]; + + const utxos = [ + { + outputID: 7, + txid: '2NWd9hrSGkWJWyTu4DnSM1qQSUT2DVm8uqwFk4wQRFLAmcsHQz', + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '100000000000000', + threshold: 2, + addresses, + locktime: '0', + }, + ]; + + // First delegation + const builder1 = factory.getDelegatorBuilder(); + const delegation1EndTime = now + 60 + MIN_DELEGATION_DURATION; + + const tx1 = await builder1 + .nodeID(validNodeID) + .startTime(now + 60) + .endTime(delegation1EndTime) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + // Second delegation starts right after first one ends + const builder2 = factory.getDelegatorBuilder(); + const utxos2 = [ + { + outputID: 7, + txid: '2K9rgiEgsEcT3dB9EyduEHFq7g4sHQBMks16VUQArVRieBQ9W5', // Valid CB58 txid + outputidx: '0', + assetID: CONTEXT.avaxAssetID, + amount: '50010000000000', // 50010 FLR (50k stake + extra for fees) + threshold: 2, + addresses, + locktime: '0', + }, + ]; + + const tx2 = await builder2 + .nodeID(validNodeID) + .startTime(delegation1EndTime + 1) // Starts 1 second after first ends + .endTime(delegation1EndTime + 1 + MIN_DELEGATION_DURATION) + .stakeAmount(MIN_DELEGATION_AMOUNT) + .fromPubKey(addresses) + .decodedUtxos(utxos2) + .feeState(mockFeeState) + .context(CONTEXT as any) + .build(); + + tx1.should.be.instanceof(Transaction); + tx2.should.be.instanceof(Transaction); + tx1.id.should.not.equal(tx2.id); + }); + }); });