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.

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

Add non-breaking muxed account support to fee-bump transactions. #434

Merged
merged 9 commits into from Jun 9, 2021
4 changes: 4 additions & 0 deletions CHANGELOG.md
@@ -1,7 +1,11 @@
# Changelog


## Unreleased

### Add
- **Opt-in support for muxed accounts.** In addition to the support introduced in [v5.2.0](https://github.com/stellar/js-stellar-base/releases/v5.2.0), this completes support for muxed accounts by enabling them for fee-bump transactions. Pass the muxing ID as the (new, optional) last parameter to `TransactionBuilder.buildFeeBumpTransaction` to make the `feeSource` a fully-muxed account instance ([#434](https://github.com/stellar/js-stellar-base/pull/434)).


## [v5.2.0](https://github.com/stellar/js-stellar-base/compare/v5.1.0..v5.2.0)

Expand Down
21 changes: 21 additions & 0 deletions src/account.js
Expand Up @@ -149,6 +149,17 @@ export class MuxedAccount {
return new MuxedAccount(new Account(gAddress, sequenceNum), id);
}

/**
* A helper method to turn an M-address into its underlying G-address.
*
* @param {string} mAddress - muxed address to convert
* @returns {string} underlying G-address
*/
static parseBaseAddress(mAddress) {
Copy link
Member

Choose a reason for hiding this comment

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

The name of this function is a little confusing. Often functions named parseX are understood to "parse X". In this case the function is named parseBaseAddress but what it does is "parse an M address".

I think it would be easier to understand the intent of this function if it was named parseMuxedAddress and it returned all the components of a muxed address. I thought we would have had something like this already, but I guess we don't?

Copy link
Contributor Author

@Shaptic Shaptic Jun 7, 2021

Choose a reason for hiding this comment

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

I see what you're saying. We do have that: MuxedAccount.fromAddress, but doing M -> G strings becomes unwieldy:

const muxed = new MuxedAccount.fromAddress(mAddress, '0');
const gAddress = muxed.baseAccount().accountId();

(see cfc6df0). How about extract over parse, @leighmcculloch? e.g.

const gAddress = MuxedAccount.extractBaseAddress(mAddress);

const muxedAccount = decodeAddressToMuxedAccount(mAddress, true);
return encodeMuxedAccountToAddress(muxedAccount, false);
}

/**
* @return {Account} the underlying account object shared among all muxed
* accounts with this Stellar address
Expand Down Expand Up @@ -195,6 +206,16 @@ export class MuxedAccount {
return this.account.incrementSequenceNumber();
}

/**
* Creates another muxed "sub"account from the base with a new ID set
*
* @param {string} id - the ID of the new muxed account
* @return {MuxedAccount} a new instance w/ the specified parameters
*/
createSubaccount(id) {
Copy link
Member

Choose a reason for hiding this comment

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

If we need this method can we rename this to createdMuxedAccount? See #433 for why.

However, I think this function confusing. You can't create a muxed account that multiplexes another muxed account. The following code, although verbose, seems much clearer. Is there a reason we can't use it? or a reason we need to be able to create muxed accounts from muxed accounts?

muxedAccount.baseAccount().createMuxedAccount(id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though I completely agree that it's odd to have this method on this object, I added it to MuxedAccount so that there was "interface" compatibility with Account. And 👍 for the rename, though unfortunate that it wasn't noted at its introduction.

Copy link
Member

Choose a reason for hiding this comment

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

What is driving the need for this interface compatibility? I don't see anything new calling createSubaccount in this PR.

Copy link
Contributor Author

@Shaptic Shaptic Jun 4, 2021

Choose a reason for hiding this comment

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

There's no true need (especially since nobody is using muxed accounts); I just had it in mind after stellar/js-stellar-sdk#653.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting. In this case, I don't think all accounts should have this capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree in theory, but then you cannot use MuxedAccount abstraction where Account is allowed, yet the two should be largely interchangeable. Fixing this imo fundamentally requires a bigger change (see this comment, where createSubaccount exists only on the non-interface), but that's out of scope here, so for now I think making muxed accounts "quack like an Account" is a reasonable measure.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for explaining.

For my own understanding, which is limited here, is there no way in JavaScript/TypeScript to specify an interface type that provides a set of functions, so AccountResponse and MuxedAccount don't always have to have the exact same function set as Account, but rather there could be some interface Account that is a common subset that doesn't include createSubaccount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're essentially describing exactly what I want: an interface that covers the common subset of functionality across AccountResponse, MuxedAccount, and Account. This would involve replacing everywhere that says Account (an object) which used to act as said "common subset" with some new interface.

return new MuxedAccount(this.account, id);
}

/**
* @return {xdr.MuxedAccount} the XDR object representing this muxed account's
* G-address and uint64 ID
Expand Down
18 changes: 14 additions & 4 deletions src/fee_bump_transaction.js
Expand Up @@ -13,12 +13,19 @@ import { encodeMuxedAccountToAddress } from './util/decode_encode_muxed_account'
* Once a {@link FeeBumpTransaction} has been created, its attributes and operations
* should not be changed. You should only add signatures (using {@link FeeBumpTransaction#sign}) before
* submitting to the network or forwarding on to additional signers.
* @param {string|xdr.TransactionEnvelope} envelope - The transaction envelope object or base64 encoded string.
* @param {string} networkPassphrase passphrase of the target stellar network (e.g. "Public Global Stellar Network ; September 2015").
*
* @param {string|xdr.TransactionEnvelope} envelope - transaction envelope
* object or base64 encoded string.
* @param {string} networkPassphrase - passphrase of the target Stellar network
* (e.g. "Public Global Stellar Network ; September 2015").
* @param {bool} [opts.withMuxing] - indicates that the fee source of this
* transaction is a proper muxed account (i.e. coming from an M... address).
* By default, this option is disabled until muxed accounts are mature.
*
* @extends TransactionBase
*/
export class FeeBumpTransaction extends TransactionBase {
constructor(envelope, networkPassphrase) {
constructor(envelope, networkPassphrase, withMuxing) {
if (typeof envelope === 'string') {
const buffer = Buffer.from(envelope, 'base64');
envelope = xdr.TransactionEnvelope.fromXDR(buffer);
Expand All @@ -42,7 +49,10 @@ export class FeeBumpTransaction extends TransactionBase {
const innerTxEnvelope = xdr.TransactionEnvelope.envelopeTypeTx(
tx.innerTx().v1()
);
this._feeSource = encodeMuxedAccountToAddress(this.tx.feeSource());
this._feeSource = encodeMuxedAccountToAddress(
this.tx.feeSource(),
withMuxing
);
this._innerTransaction = new Transaction(
innerTxEnvelope,
networkPassphrase
Expand Down
32 changes: 30 additions & 2 deletions src/keypair.js
@@ -1,9 +1,13 @@
import nacl from 'tweetnacl';
import isUndefined from 'lodash/isUndefined';
import isString from 'lodash/isString';

import { sign, verify, generate } from './signing';
import { StrKey } from './strkey';
import xdr from './generated/stellar-xdr_generated';
import { hash } from './hashing';

import xdr from './generated/stellar-xdr_generated';

/**
* `Keypair` represents public (and secret) keys of the account.
*
Expand Down Expand Up @@ -121,7 +125,31 @@ export class Keypair {
return new xdr.PublicKey.publicKeyTypeEd25519(this._publicKey);
}

xdrMuxedAccount() {
/**
* Creates a {@link xdr.MuxedAccount} object from the public key.
*
* You will get a different type of muxed account depending on whether or not
* you pass an ID.
*
* @param {string} [id] - stringified integer indicating the underlying muxed
* ID of the new account object
*
* @return {xdr.MuxedAccount}
*/
xdrMuxedAccount(id) {
if (!isUndefined(id)) {
if (!isString(id)) {
throw new TypeError(`expected string for ID, got ${typeof id}`);
}

return xdr.MuxedAccount.keyTypeMuxedEd25519(
new xdr.MuxedAccountMed25519({
id: xdr.Uint64.fromString(id),
ed25519: this._publicKey
})
);
}

return new xdr.MuxedAccount.keyTypeEd25519(this._publicKey);
}

Expand Down
2 changes: 1 addition & 1 deletion src/operations/payment.js
Expand Up @@ -35,7 +35,7 @@ export function payment(opts) {
opts.withMuxing
);
} catch (e) {
throw new Error('destination is invalid');
throw new Error('destination is invalid; did you forget to enable muxing?');
}

attributes.asset = opts.asset.toXDRObject();
Expand Down
38 changes: 30 additions & 8 deletions src/transaction_builder.js
Expand Up @@ -272,18 +272,36 @@ export class TransactionBuilder {
}

/**
* Builds a {@link FeeBumpTransaction}
* @param {Keypair} feeSource - The account paying for the transaction.
* @param {string} baseFee - The max fee willing to pay per operation in inner transaction (**in stroops**). Required.
* @param {Transaction} innerTx - The Transaction to be bumped by the fee bump transaction.
* @param {string} networkPassphrase - networkPassphrase of the target stellar network (e.g. "Public Global Stellar Network ; September 2015").
* Builds a {@link FeeBumpTransaction}, enabling you to resubmit an existing
* transaction with a higher fee.
*
* @param {Keypair} feeSource - account paying for the transaction
* @param {string} baseFee - max fee willing to pay per
* operation in inner transaction (**in stroops**)
* @param {Transaction} innerTx - {@link Transaction} to be bumped
* by the fee bump transaction
* @param {string} networkPassphrase - passphrase of the target Stellar
* network (e.g. "Public Global Stellar Network ; September 2015")
* @param {string} [muxedId] - an (optional, stringified) ID to
* indicate that the source account for the fee-bump is a muxed account.
* The muxed account will be built from the `feeSource`'s public key and
* this `muxedId`.
*
* @todo Alongside the next major version bump, this type signature can be
* changed to be less awkward: accept a MuxedAccount as the `feeSource`
* rather than a keypair and an optional ID.
*
* @note Your fee-bump amount should be 10x the original fee.
* @see https://developers.stellar.org/docs/glossary/fee-bumps/#replace-by-fee
*
* @returns {FeeBumpTransaction}
*/
static buildFeeBumpTransaction(
feeSource,
baseFee,
innerTx,
networkPassphrase
networkPassphrase,
muxedId
leighmcculloch marked this conversation as resolved.
Show resolved Hide resolved
) {
const innerOps = innerTx.operations.length;
const innerBaseFeeRate = new BigNumber(innerTx.fee).div(innerOps);
Expand Down Expand Up @@ -328,7 +346,7 @@ export class TransactionBuilder {
}

const tx = new xdr.FeeBumpTransaction({
feeSource: feeSource.xdrMuxedAccount(),
feeSource: feeSource.xdrMuxedAccount(muxedId),
fee: xdr.Int64.fromString(base.mul(innerOps + 1).toString()),
innerTx: xdr.FeeBumpTransactionInnerTx.envelopeTypeTx(
innerTxEnvelope.v1()
Expand All @@ -343,7 +361,11 @@ export class TransactionBuilder {
feeBumpTxEnvelope
);

return new FeeBumpTransaction(envelope, networkPassphrase);
return new FeeBumpTransaction(
envelope,
networkPassphrase,
!isUndefined(muxedId)
);
}

/**
Expand Down
55 changes: 52 additions & 3 deletions test/unit/transaction_builder_test.js
Expand Up @@ -680,10 +680,11 @@ describe('TransactionBuilder', function() {
);

const PUBKEY_SRC = StellarBase.StrKey.decodeEd25519PublicKey(
'GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ'
source.baseAccount().accountId()
);
const MUXED_SRC_ID = StellarBase.xdr.Uint64.fromString('2');
const networkPassphrase = 'Standalone Network ; February 2017';
const signer = StellarBase.Keypair.master(StellarBase.Networks.TESTNET);

it('enables muxed support after creation', function() {
let builder = new StellarBase.TransactionBuilder(source, {
Expand All @@ -707,7 +708,6 @@ describe('TransactionBuilder', function() {
// TODO: More muxed-enabled operations
];

const signer = StellarBase.Keypair.master(StellarBase.Networks.TESTNET);
let builder = new StellarBase.TransactionBuilder(source, {
fee: '100',
timebounds: { minTime: 0, maxTime: 0 },
Expand All @@ -726,7 +726,7 @@ describe('TransactionBuilder', function() {
tx.sign(signer);

const envelope = tx.toEnvelope();
const xdrTx = envelope.v1().tx();
const xdrTx = envelope.value().tx();

const rawMuxedSourceAccount = xdrTx.sourceAccount();

Expand All @@ -753,5 +753,54 @@ describe('TransactionBuilder', function() {
);
}).to.not.throw();
});

it('works with fee-bump transactions', function() {
// We create a non-muxed transaction, then fee-bump with a muxed source.
let builder = new StellarBase.TransactionBuilder(source.baseAccount(), {
fee: '100',
timebounds: { minTime: 0, maxTime: 0 },
networkPassphrase: networkPassphrase
});

builder.addOperation(
StellarBase.Operation.payment({
source: source.baseAccount().accountId(),
destination: StellarBase.MuxedAccount.parseBaseAddress(destination),
amount: amount,
asset: asset
})
);

let tx = builder.build();
tx.sign(signer);

const kp = StellarBase.Keypair.fromPublicKey(
source.baseAccount().accountId()
);
const feeTx = StellarBase.TransactionBuilder.buildFeeBumpTransaction(
kp,
'1000',
tx,
networkPassphrase,
'2'
);

expect(feeTx).to.be.an.instanceof(StellarBase.FeeBumpTransaction);
const envelope = feeTx.toEnvelope();
const xdrTx = envelope.value().tx();

const rawFeeSource = xdrTx.feeSource();

expect(rawFeeSource.switch()).to.equal(
StellarBase.xdr.CryptoKeyType.keyTypeMuxedEd25519()
);

const innerMux = rawFeeSource.med25519();
expect(innerMux.ed25519()).to.eql(PUBKEY_SRC);
expect(encodeMuxedAccountToAddress(rawFeeSource, true)).to.equal(
source.accountId()
);
expect(innerMux.id()).to.eql(MUXED_SRC_ID);
});
});
});
17 changes: 14 additions & 3 deletions types/index.d.ts
Expand Up @@ -16,11 +16,13 @@ export class Account {
export class MuxedAccount {
constructor(account: Account, sequence: string);
static fromAddress(mAddress: string, sequenceNum: string): MuxedAccount;
static parseBaseAddress(mAddress: string): string;

/* Modeled after Account, above */
accountId(): string;
sequenceNumber(): string;
incrementSequenceNumber(): void;
createSubaccount(id: string): MuxedAccount;

baseAccount(): Account;
id(): string;
Expand Down Expand Up @@ -99,7 +101,10 @@ export class Keypair {
signDecorated(data: Buffer): xdr.DecoratedSignature;
signatureHint(): Buffer;
verify(data: Buffer, signature: Buffer): boolean;

xdrAccountId(): xdr.AccountId;
xdrPublicKey(): xdr.PublicKey;
xdrMuxedAccount(id: string): xdr.MuxedAccount;
}

export const MemoNone = 'none';
Expand Down Expand Up @@ -763,7 +768,8 @@ export class TransactionI {
export class FeeBumpTransaction extends TransactionI {
constructor(
envelope: string | xdr.TransactionEnvelope,
networkPassphrase: string
networkPassphrase: string,
withMuxing?: boolean
);
feeSource: string;
innerTransaction: Transaction;
Expand All @@ -775,7 +781,8 @@ export class Transaction<
> extends TransactionI {
constructor(
envelope: string | xdr.TransactionEnvelope,
networkPassphrase: string
networkPassphrase: string,
withMuxing?: boolean
);
memo: TMemo;
operations: TOps;
Expand Down Expand Up @@ -804,12 +811,15 @@ export class TransactionBuilder {
feeSource: Keypair,
baseFee: string,
innerTx: Transaction,
networkPassphrase: string
networkPassphrase: string,
id?: string
): FeeBumpTransaction;
static fromXDR(
envelope: string | xdr.TransactionEnvelope,
networkPassphrase: string
): Transaction | FeeBumpTransaction;

supportMuxedAccounts: boolean;
}

export namespace TransactionBuilder {
Expand All @@ -822,6 +832,7 @@ export namespace TransactionBuilder {
memo?: Memo;
networkPassphrase?: string;
v1?: boolean;
withMuxing?: boolean;
}
}

Expand Down
2 changes: 2 additions & 0 deletions types/test.ts
Expand Up @@ -6,6 +6,8 @@ const destKey = StellarSdk.Keypair.random();
const usd = new StellarSdk.Asset('USD', 'GDGU5OAPHNPU5UCLE5RDJHG7PXZFQYWKCFOEXSXNMR6KRQRI5T6XXCD7'); // $ExpectType Asset
const account = new StellarSdk.Account(sourceKey.publicKey(), '1'); // $ExpectType Account
const muxedAccount = new StellarSdk.MuxedAccount(account, '123'); // $ExpectType MuxedAccount
const muxedConforms = muxedAccount as StellarSdk.Account; // $ExpectType Account

const transaction = new StellarSdk.TransactionBuilder(account, {
fee: "100",
networkPassphrase: StellarSdk.Networks.TESTNET
Expand Down