Turn on support for muxed addresses by default. #485

Shaptic · 2022-01-06T07:59:57Z

see stellar/go#4164 for context

marcelosalloum

I've added a few suggestions and highlighted one breaking change❗about the base fee.

Please feel free to merge when you think it's ready.

src/operation.js

src/operations/account_merge.js

src/operations/path_payment_strict_send.js

src/transaction_builder.js

test/unit/operation_test.js

Shaptic · 2022-01-10T21:12:38Z

@marcelosalloum new requirements rolled in: We are not going to allow "opting out" of muxing support. Basically, the default for all of the withMuxing, supportMuxing, etc. is now true and there's no way to pass false. The latest set of pushes will do this. I've resolved your comments that talk about the flags since they're just gone now.

Please take another look!

There is ONE case in which the above (no opt-out) will not hold:

function encodeMuxedAccountToAddress(muxedAccount, forceEd25519) {}

The purpose of leaving it here is because sometimes you only want to extract the underlying G... address when you have an xdr.MuxedAccount option. Most notably, this is done by the MuxedAccount.fromAddress helper function. I was considering moving this specific functionality to a separate helper (e.g. encodeMuxedAccountAsEd25519) to separate it, but maybe that's excessive. Wdyt?

src/muxed_account.js

leighmcculloch

Really excited to see this happening, and I like that we're going with the unconditional support route.

I have some questions and concerns inline. In a few places it feels like this PR is doing multiple things such as refactoring areas of the code and fixing other unrelated issues/cleanup. It is possible they are all related and I don't see the connections. Could this PR be hyperfocused on the functional changes required for muxed address by default?

src/muxed_account.js

src/operation.js

src/operations/clawback.js

src/transaction_builder.js

leighmcculloch · 2022-01-10T22:52:14Z

src/util/decode_encode_muxed_account.js

 *
 * @returns {string}  stringified G... (corresponding to the underlying pubkey)
 *     or M... address (corresponding to both the key and the muxed ID)
 */
-export function encodeMuxedAccountToAddress(muxedAccount, supportMuxing) {
+export function encodeMuxedAccountToAddress(muxedAccount, forceEd25519) {


Do we need to keep the forceEd25519? This function is named encodedMuxedAccountToAddress, and the only valid address for a muxed account is a M address. If someone wants to encode the base account of a muxed accounmt to a G address, they should probably call mux.baseAccount() and go from there?

Is this required for compatibility?

It's a separation of concerns: these functions are for operating between xdr.MuxedAccount <--> string, and then there's also the higher-level MuxedAccount object that implements the Account interface.

I touched on this a bit above (#485 (comment)), but basically we need to be able to forceEd25519 when actually building a MuxedAccount object from an M-address in order to get its composite G-address and ID.

Ah I see. I still think we should follow a deconstruct and a construct approach, rather than a parameter that makes a construction behave differently. By deconstruct I mean get the base address/account id out of the xdr.MuxedAccount. We should probably have a function on MuxedAccount to do this if we don't already.

Let me know what I'm missing if I'm way off here.

I wrote up an overly-verbose comment on the design decisions I was making and all of the abstraction layers we need to deal with (the TL;DR is that we have xdr.MuxedAccount ↔️ M-address, xdr.MuxedAccount ↔️ G-address, and MuxedAccount ↔️ M-address), but I think the latest commit explains all that is needed: 10f14a3.

I've dropped the forceEd25519 flag and moved that subset of functionality to a new helper,

function extractBaseAddress(mAddress: string): string;

Does that help?

Shaptic · 2022-01-10T23:17:03Z

Thanks for your review, @leighmcculloch! The latest push should hyper-focus on the new muxing defaults delta.

…ddress

leighmcculloch

Looks great 👏🏻.

One suggested doc change (💡), very minor. And one question about how a new function will be used in one scenario (❔).

src/util/decode_encode_muxed_account.js

leighmcculloch · 2022-01-11T18:44:13Z

src/util/decode_encode_muxed_account.js

+  if (!StrKey.isValidMed25519PublicKey(address)) {
+    throw new TypeError('address should be a muxed account (M...)');
+  }


❔ Is there a way to test if an address is a muxed address before passing it to this function? I'm wondering about the flow where someone has a value that might be an M or a G and they want to get the G. If they call this function on its own it might error in the G cases. How do they distinguish? Do they just check the first character? Or maybe this function should just return the G if the input is a G. I don't feel strongly what way we go, but curious what the story is for this use case.

Haha, I realize the line I'm commenting on literally has the function for testing if an address is a muxed address.

I still wonder if there is value in not erroring in the case a G is passed and just returning the G. It might make the API a little easier to use, albeit with no errors in a case that might be error worthy.

That's an interesting question. I can't foresee when it would come up, but I don't have a strong opinion either way.

Callers can use the StrKey validators (isValidEd25519PublicKey and isValidMed25519PublicKey, just like is done here) before calling the function. I added a check in 9e1ee1f to just pass G-addresses through, for convenience.

Hah, should have refreshed before replying 🥷. But yeah, added the pass-through.

Shaptic added 12 commits January 5, 2022 11:44

Turn muxing on some operations by default

a0bc1ff

Bugfix: let clawbacks control muxing support

742a239

Make AccountMergeOp enable muxing by default

c6ca4e4

Make PaymentOp enable muxing by default

62d8ed6

Make PathPaymentStrict[Send|Receive] enable muxing by default

b69581d

Make operation source account allow muxing by default

e433768

Make transactions support muxing by default

fdea102

Update all tests to expect default=on instead of default=off

bd4a98e

Turn muxing on when decoding by default

766248c

Fix docstrings and messages where appropriate

f08b196

Update fee-bump test to now expect muxed by default

c1c9864

Update account merge tests to expect muxed by default

a7f02e9

Shaptic requested review from marcelosalloum and a team January 6, 2022 07:59

marcelosalloum approved these changes Jan 7, 2022

View reviewed changes

Shaptic added 9 commits January 10, 2022 11:02

Drop opt-out for the encode/decode helpers

a418e33

Break Account and MuxedAccount apart into separate files

f169813

Update strkey tests to incorporate new flag options

82c29fa

Update muxed account test to incorporate new API

9b523fa

Remove muxing flag from all operations

7d04cb9

Stop passing muxing flag to operation decoders

fd9dbd2

Update operation tests to handle new muxing defaults

5de8ead

Drop muxing flag from transaction + its builder

c5335c5

Update all test cases to prevent disabling muxing

ff24580

Shaptic force-pushed the muxed-on branch from 1ae9c7a to ff24580 Compare January 10, 2022 21:04

Shaptic requested a review from marcelosalloum January 10, 2022 21:12

Shaptic commented Jan 10, 2022

View reviewed changes

src/muxed_account.js Outdated Show resolved Hide resolved

Shaptic mentioned this pull request Jan 10, 2022

Rename createSubaccount to createMuxedAccount #433

Closed

Drop the AccountInterface.createSubaccount() requirement

7b9acde

Add changelog entry

dbac14c

leighmcculloch reviewed Jan 10, 2022

View reviewed changes

Shaptic added 6 commits January 10, 2022 15:01

Drop irrelevant docstring

2b53c9e

Remove MuxedAccount.createSibling

e9610c0

Put MuxedAccount back into account.js to simplify the diff

9fe601a

Remove the empty file

2f7da08

Update changelog entry

f785881

Add back old txfee handling

2bbc510

Drop createSubaccount removal in lieu of separate-muxed-accounts branch

00ce1a8

Shaptic requested a review from leighmcculloch January 10, 2022 23:22

Shaptic added 6 commits January 10, 2022 15:22

Update changelog entry

a9a32a4

Stop passing muxing flag with fee-bumps

ab308df

Separate flagged encodeMuxedAccountToAddress behavior to extractBaseA…

10f14a3

…ddress

Update TS definitions to drop muxing flag

c5104ac

Update TS test to match definitions

05a4b04

Simplify docstrings

d51c286

leighmcculloch approved these changes Jan 11, 2022

View reviewed changes

Pass through G-addresses when extracting

9e1ee1f

Shaptic merged commit 510fcd4 into master Jan 11, 2022

Shaptic deleted the muxed-on branch January 11, 2022 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on support for muxed addresses by default. #485

Turn on support for muxed addresses by default. #485

Shaptic commented Jan 6, 2022

marcelosalloum left a comment

Shaptic commented Jan 10, 2022

leighmcculloch left a comment

leighmcculloch Jan 10, 2022

Shaptic Jan 10, 2022

leighmcculloch Jan 10, 2022 •

edited

Loading

Shaptic Jan 11, 2022

Shaptic commented Jan 10, 2022

leighmcculloch left a comment

leighmcculloch Jan 11, 2022

leighmcculloch Jan 11, 2022

leighmcculloch Jan 11, 2022

Shaptic Jan 11, 2022

Shaptic Jan 11, 2022

Turn on support for muxed addresses by default. #485

Turn on support for muxed addresses by default. #485

Conversation

Shaptic commented Jan 6, 2022

marcelosalloum left a comment

Choose a reason for hiding this comment

Shaptic commented Jan 10, 2022

leighmcculloch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leighmcculloch Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaptic commented Jan 10, 2022

leighmcculloch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leighmcculloch Jan 10, 2022 •

edited

Loading