txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527

2opremio · 2021-04-06T14:11:13Z

This is ~~an initial RFC draft~~ a PR to re-introduce Multiplexed addresses in txnbuild, considering all the requirements from #3490 .

My main criteria for this proposal has been that no user should be enabling M-addresss unknowingly, either for new or existing code. For that reason:

New public functions/methods enabling SEP23 state so explicitly in their name
Existing public functions/methods keep their name and enable SEP23 through explicit options

Key changes:

the txnbuild.Operation interface now adds a withMuxedAccount parameter to most of its methods:

    type Operation interface {
	BuildXDR(withMuxedAccounts bool) (xdr.Operation, error)
	FromXDR(xdrOp xdr.Operation, withMuxedAccounts bool) error
	Validate(withMuxedAccounts bool) error
	GetSourceAccount() string
}

~~For now, only txnbuild.AccountMerge supports the extended interface.~~

NewTransaction(), TransactionFromXDR() and NewFeeBumpTransaction() now accept an option to enable SEP23, while keeping backwards compatibility

TODO:

Implement support for the new SEP23-methods in all operations
Implement support for the updated M-strkey structure from SEP-23: Propose {ed25519}{id} for look-a-like G and M addresses stellar-protocol#895
Add tests
~~Add support for SEP23 in the the SEP10 functions (BuildChallengeTx(), ReadChallengeTx() ...). I am not even sure we should, TBH (I would appreciate feedback on this).~~ We agreed offline not to do it.

txnbuild/transaction.go

txnbuild/operation.go

strkey/main.go

txnbuild/account_merge.go

txnbuild/operation.go

txnbuild/account_merge.go

txnbuild/allow_trust.go

2opremio · 2021-04-09T10:39:36Z

@tamirms Operation-tests (which are cooking) and #3527 (comment) aside, this is ready for review. PTAL while I am on it so that we can reach the release quicker.

Same for @bartekn (but I know he is off today :) ).

strkey/main.go

txnbuild/clawback.go

tamirms · 2021-04-12T08:01:51Z

txnbuild/path_payment.go

 	result, ok := xdrOp.Body.GetPathPaymentStrictReceiveOp()
 	if !ok {
 		return errors.New("error parsing path_payment operation from xdr")
 	}

-	pp.SourceAccount = accountFromXDR(xdrOp.SourceAccount)
+	pp.SourceAccount = accountFromXDR(xdrOp.SourceAccount, withMuxedAccounts)
+	if withMuxedAccounts {


I think accountFromXDR can be used to assign to pp.Destination

Not really, because accountFromXDR() accepts nil values.

txnbuild/transaction.go

leighmcculloch

Really exciting to see this rolling.

I left comments about a couple things I think we should change (❗).

If you're open, I also left some suggestions (💡) that are to do with how the API is changing for the user, and how I think we could make the API is easier to understand.

txnbuild/account_merge.go

txnbuild/claim_claimable_balance.go

txnbuild/transaction.go

xdr/muxed_account.go

2opremio · 2021-04-14T00:10:59Z

It took considerable effort (due to the lack of prior coverage) but all operations should now be tested.

This should be good to go. @tamirms @bartekn please take a final look.

xdr/muxed_account.go

txnbuild/CHANGELOG.md

bartekn · 2021-04-14T14:14:32Z

txnbuild/account_merge.go

@@ -13,10 +13,14 @@ type AccountMerge struct {
 }

 // BuildXDR for AccountMerge returns a fully configured XDR Operation.
-func (am *AccountMerge) BuildXDR() (xdr.Operation, error) {
+func (am *AccountMerge) BuildXDR(withMuxedAccounts bool) (xdr.Operation, error) {


Nit: I know this PR has been reviewed multiple times but I have one (simple) idea how to improve the API. Passing bool value to methods can be misleading (user need to check the description of the method to understand what is the param). We can create additional extra methods for each method accepting withMuxedAccounts. BuildXDR as an example:

Change BuildXDR(withMuxedAccounts bool) (xdr.Operation, error) to buildXDR(withMuxedAccounts bool) (xdr.Operation, error)

BuildXDR() stays in a form before this PR. Calls buildXDR(false) internally.

A new BuildXDRWithMuxedAccounts() is added and calls buildXDR(true) internally.

@bartekn that's exactly what I implemented originally and I discarded after the design discussion. Please see 27a79c6#diff-0ef2e7d165d2f196939c937afadef91206823b0671f32120b265843fd291332aR15 and the issue body edit history (which originally presented the same design you suggested, symbol-naming aside).

We should be more cohesive when discussing designs (but that's for a retro discussion). The purpose of creating an early draft a week ago was exactly avoiding this type of situation.

I personally think this needs to go out (it's already late) but if @ire-and-curses thinks it's worth another rewrite, I will cave in and restore my original design.

@ire-and-curses In order to get more context on what happened:

I originally proposed what @bartekn is proposing now (you can go through the edit history of the issue body and see it).

@tamirms suggested using an internal boolean (appended to each operation struct) at txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527 (comment)

We discarded it at txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527 (comment) and went for the design as it is now.

I proceed to implement the design for all the operations.

@leighmcculloch proposed yet another design modification at txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527 (comment) which I discarded.

@bartekn suggests my original design.

Profit??!!
Let's discuss in team meeting today. I really want to get this out.

We decided offline to keep it as it is. Thanks for the suggestion though @bartekn

Feature summary: - Operations updated to support muxed accounts: * Operation.sourceAccount * Payment.destination * PathPaymentStrictReceive.destination * PathPaymentStrictSend.destination * AccountMerge.destination * Transaction.sourceAccount - M... addresses can be converted to & from their XDR representation easily - Muxed accounts can be created & managed efficiently w/ helpers (MuxedAccount) - Feature is hidden behind an opt-in flag (`withMuxing` boolean) Limitations: - The final missing implementation detail is adapting `FeeBumpTransaction.feeSource` to accept an M-address, but this will not (and _should not_) be a part of this pull request because it involves a breaking change. Currently, `TransactionBuilder.buildFeeBumpTransaction` takes `feeSource` typed to `Keypair`, which doesn't support M-addresses. Either Keypair will need to support M-addresses (unlikely, as this breaks abstraction barriers) or the type will need to change (likely), so this will be part of a breaking 6.0 release. See #422. More reading: - CAP-27 (muxed account XDR), - SEP-23 (M-addresses), - stellar/stellar-protocol#617 (rollout plan) - stellar/go#3527 (Go SDK reference impl.)

2opremio requested review from leighmcculloch, tomerweller and a team April 6, 2021 14:11

tamirms reviewed Apr 7, 2021

View reviewed changes

txnbuild/transaction.go Outdated Show resolved Hide resolved

tamirms reviewed Apr 7, 2021

View reviewed changes

txnbuild/transaction.go Outdated Show resolved Hide resolved

tamirms reviewed Apr 7, 2021

View reviewed changes

txnbuild/operation.go Outdated Show resolved Hide resolved

bartekn reviewed Apr 7, 2021

View reviewed changes

strkey/main.go Outdated Show resolved Hide resolved

txnbuild/account_merge.go Outdated Show resolved Hide resolved

txnbuild/operation.go Outdated Show resolved Hide resolved

Shaptic reviewed Apr 7, 2021

View reviewed changes

txnbuild/account_merge.go Outdated Show resolved Hide resolved

tamirms reviewed Apr 7, 2021

View reviewed changes

txnbuild/allow_trust.go Show resolved Hide resolved

2opremio marked this pull request as ready for review April 7, 2021 22:49

This was referenced Apr 8, 2021

strkey: Check for leftover character to be in the expected alphabet #3529

Merged

SEP-23: Propose {ed25519}{id} for look-a-like G and M addresses stellar/stellar-protocol#895

Merged

tamirms reviewed Apr 12, 2021

View reviewed changes

strkey/main.go Outdated Show resolved Hide resolved

tamirms reviewed Apr 12, 2021

View reviewed changes

txnbuild/clawback.go Outdated Show resolved Hide resolved

tamirms reviewed Apr 12, 2021

View reviewed changes

txnbuild/transaction.go Outdated Show resolved Hide resolved

tamirms approved these changes Apr 12, 2021

View reviewed changes

leighmcculloch reviewed Apr 12, 2021

View reviewed changes

2opremio added 11 commits April 14, 2021 02:04

Re-enable M-key parsing in strkey package

efb9c3b

Enable SEP23 parsing for MuxedAccounts (with separated functions)

a0bf286

WIP: Add flag-protected support for SEP23 in txnbuild

27a79c6

Fix comment typos

14ec0cb

Address review feedback

8546938

Address review comments

f862efb

WIP: port more oeprations to the new interface

0742e88

Update all the remaining operations

3533829

Appease go vet

d8e1e2a

Add CHANGELOG entry

29bf10c

Update client memo check to support Muxed accounts

828be10

2opremio added 4 commits April 14, 2021 02:04

Add more tests

265b601

Add client test

85a33de

Fix problems after auto-review

afa16c3

Add operation tests

0a6beda

2opremio force-pushed the 3490-reenable-muptiplexed-addresses branch from ed74412 to 0a6beda Compare April 14, 2021 00:04

Rename option in the changelog

4bad428

tamirms approved these changes Apr 14, 2021

View reviewed changes

bartekn approved these changes Apr 14, 2021

View reviewed changes

Address review feedback

8caa781

2opremio merged commit a9f3903 into stellar:master Apr 14, 2021

2opremio deleted the 3490-reenable-muptiplexed-addresses branch April 14, 2021 20:12

Shaptic mentioned this pull request Apr 22, 2021

Add muxed account (SEP-23) support stellar/js-stellar-base#416

Merged

16 tasks

Shaptic mentioned this pull request Jan 6, 2022

txnbuild: Support muxed addresses (SEP-23) by default. #4164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527

txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527

2opremio commented Apr 6, 2021 •

edited

Loading

2opremio commented Apr 9, 2021 •

edited

Loading

tamirms Apr 12, 2021

2opremio Apr 12, 2021

leighmcculloch left a comment

2opremio commented Apr 14, 2021

bartekn Apr 14, 2021

2opremio Apr 14, 2021 •

edited

Loading

2opremio Apr 14, 2021 •

edited

Loading

ire-and-curses Apr 14, 2021

2opremio Apr 14, 2021

txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527

txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527

Conversation

2opremio commented Apr 6, 2021 • edited Loading

2opremio commented Apr 9, 2021 • edited Loading

tamirms Apr 12, 2021

Choose a reason for hiding this comment

2opremio Apr 12, 2021

Choose a reason for hiding this comment

leighmcculloch left a comment

Choose a reason for hiding this comment

2opremio commented Apr 14, 2021

bartekn Apr 14, 2021

Choose a reason for hiding this comment

2opremio Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

2opremio Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

ire-and-curses Apr 14, 2021

Choose a reason for hiding this comment

2opremio Apr 14, 2021

Choose a reason for hiding this comment

2opremio commented Apr 6, 2021 •

edited

Loading

2opremio commented Apr 9, 2021 •

edited

Loading

2opremio Apr 14, 2021 •

edited

Loading

2opremio Apr 14, 2021 •

edited

Loading