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

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Jun 4, 2021

To avoid a breaking change, I've introduced a way to set the source of a fee-bump transaction via account strings (in both G... and M... form). To enable proper muxed accounts, you pass withMuxing: true as the last parameter of TransactionBuilder.buildFeeBumpTransaction() just like everywhere else to explicitly opt-in. Namely,

const feeTx = StellarBase.TransactionBuilder.buildFeeBumpTransaction(
  feeSourceMAddress,
  bumpedFee,
  innerTx,
  networkPassphrase,
  true // optionally parses the source as a true muxed address
);

@Shaptic Shaptic requested review from a team and paulbellamy June 4, 2021 21:21
@Shaptic Shaptic self-assigned this Jun 4, 2021
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

A couple questions and thoughts shared inline.

* @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.

src/transaction_builder.js Outdated Show resolved Hide resolved
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

On a final pass I noticed a function below that has a confusing name. I defer to you on it though. Otherwise the PR looks good.

src/account.js Outdated
* @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);

@Shaptic Shaptic merged commit 4ce7cf1 into master Jun 9, 2021
@Shaptic Shaptic deleted the muxed-fee-bump branch June 9, 2021 16:54
@Shaptic
Copy link
Contributor Author

Shaptic commented Jun 14, 2021

Closes #422

Shaptic added a commit that referenced this pull request Aug 3, 2021
* Make `MuxedAccount` conform to the `Account` pseudo-interface.
* Add test that fails without muxed support for fee bumps.
* Add muxing support to fee-bump transactions and their builder
* Improve payment error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants