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

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

Merged
merged 22 commits into from
Apr 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions clients/horizonclient/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"time"

"github.com/stellar/go/txnbuild"
"github.com/stellar/go/xdr"

"github.com/manucorporat/sse"

Expand Down Expand Up @@ -47,7 +48,7 @@ func (c *Client) checkMemoRequired(transaction *txnbuild.Transaction) error {
for i, op := range transaction.Operations() {
var destination string

if err := op.Validate(); err != nil {
if err := op.Validate(true); err != nil {
return err
}

Expand All @@ -64,10 +65,15 @@ func (c *Client) checkMemoRequired(transaction *txnbuild.Transaction) error {
continue
}

// TODO: once we support M-strkeys (SEP23), also check whether the destination
// is a muxed account with a memo ID.
muxed, err := xdr.AddressToMuxedAccount(destination)
if err != nil {
return errors.Wrapf(err, "destination %v is not a valid address", destination)
}
// Skip destination addresses with a memo id because the address has a memo
// encoded within it
destinationHasMemoID := muxed.Type == xdr.CryptoKeyTypeKeyTypeMuxedEd25519

if destinations[destination] {
if destinations[destination] || destinationHasMemoID {
continue
}
destinations[destination] = true
Expand Down
66 changes: 66 additions & 0 deletions clients/horizonclient/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
"github.com/stellar/go/support/errors"
"github.com/stellar/go/support/http/httptest"
"github.com/stellar/go/txnbuild"
"github.com/stellar/go/xdr"

"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -903,6 +905,70 @@ func TestSubmitTransactionRequest(t *testing.T) {
assert.Equal(t, ErrAccountRequiresMemo, errors.Cause(err))
}

func TestSubmitTransactionRequestMuxedAccounts(t *testing.T) {
hmock := httptest.NewClient()
client := &Client{
HorizonURL: "https://localhost/",
HTTP: hmock,
}

kp := keypair.MustParseFull("SA26PHIKZM6CXDGR472SSGUQQRYXM6S437ZNHZGRM6QA4FOPLLLFRGDX")
accountID := xdr.MustAddress(kp.Address())
mx := xdr.MuxedAccount{
Type: xdr.CryptoKeyTypeKeyTypeMuxedEd25519,
Med25519: &xdr.MuxedAccountMed25519{
Id: 0xcafebabe,
Ed25519: *accountID.Ed25519,
},
}
sourceAccount := txnbuild.NewSimpleAccount(mx.Address(), int64(0))

payment := txnbuild.Payment{
Destination: kp.Address(),
Amount: "10",
Asset: txnbuild.NativeAsset{},
}

tx, err := txnbuild.NewTransaction(
txnbuild.TransactionParams{
SourceAccount: &sourceAccount,
IncrementSequenceNum: true,
Operations: []txnbuild.Operation{&payment},
BaseFee: txnbuild.MinBaseFee,
Timebounds: txnbuild.NewTimebounds(0, 10),
EnableMuxedAccounts: true,
},
)
assert.NoError(t, err)

tx, err = tx.Sign(network.TestNetworkPassphrase, kp)
assert.NoError(t, err)

// successful tx with config.memo_required not found
hmock.On(
"POST",
"https://localhost/transactions?tx=AAAAAgAAAQAAAAAAyv66vgU08yUQ8sHqhY8j9mXWwERfHC%2F3cKFSe%2FspAr0rGtO2AAAAZAAAAAAAAAABAAAAAQAAAAAAAAAAAAAAAAAAAAoAAAAAAAAAAQAAAAAAAAABAAAAAAU08yUQ8sHqhY8j9mXWwERfHC%2F3cKFSe%2FspAr0rGtO2AAAAAAAAAAAF9eEAAAAAAAAAAAErGtO2AAAAQJvQkE9UVo%2FmfFBl%2F8ZPTzSUyVO4nvW0BYfnbowoBPEdRfLOLQz28v6sBKQc2b86NUfVHN5TQVo3%2BjH4nK9wVgk%3D",
).ReturnString(200, txSuccess)

hmock.On(
"GET",
"https://localhost/accounts/GACTJ4ZFCDZMD2UFR4R7MZOWYBCF6HBP65YKCUT37MUQFPJLDLJ3N5D2/data/config.memo_required",
).ReturnString(404, notFoundResponse)

_, err = client.SubmitTransaction(tx)
assert.NoError(t, err)

// memo required - does not submit transaction
hmock.On(
"GET",
"https://localhost/accounts/GACTJ4ZFCDZMD2UFR4R7MZOWYBCF6HBP65YKCUT37MUQFPJLDLJ3N5D2/data/config.memo_required",
).ReturnJSON(200, memoRequiredResponse)

_, err = client.SubmitTransaction(tx)
assert.Error(t, err)
assert.Equal(t, ErrAccountRequiresMemo, errors.Cause(err))
}

func TestSubmitFeeBumpTransaction(t *testing.T) {
hmock := httptest.NewClient()
client := &Client{
Expand Down
89 changes: 89 additions & 0 deletions strkey/decode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@ func TestDecode(t *testing.T) {
0xec, 0x9c, 0x07, 0x3d, 0x05, 0xc7, 0xb1, 0x03,
},
},
{
Name: "MuxedAccount",
Address: "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJLK",
ExpectedVersionByte: VersionByteMuxedAccount,
ExpectedPayload: []byte{
0x3f, 0x0c, 0x34, 0xbf, 0x93, 0xad, 0x0d, 0x99,
0x71, 0xd0, 0x4c, 0xcc, 0x90, 0xf7, 0x05, 0x51,
0x1c, 0x83, 0x8a, 0xad, 0x97, 0x34, 0xa4, 0xa2,
0xfb, 0x0d, 0x7a, 0x03, 0xfc, 0x7f, 0xe8, 0x9a,
0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
},
},
{
Name: "Seed",
Address: "SBU2RRGLXH3E5CQHTD3ODLDF2BWDCYUSSBLLZ5GNW7JXHDIYKXZWHOKR",
Expand Down Expand Up @@ -85,6 +97,83 @@ func TestDecode(t *testing.T) {
// corrupted payload
_, err = Decode(VersionByteAccountID, "GA3D5KRYM6CB7OWOOOORR3Z4T7GNZLKERYNZGGA5SOAOPIFY6YQHES5")
assert.Error(t, err)

// non-canonical representation due to extra character
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJLKA")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused leftover character")
}

// non-canonical representation due to leftover bits set to 1 (some of the test strkeys are too short for a muxed account
// but they comply with the test's purpose all the same)

// 1 unused bit (length 69)
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJLH")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJUAAAAAAAAAAAACJUR")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// 4 unused bits (length 68)

// 'B' is equivalent to 0b00001
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJB")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// 'C' is equivalent to 0b00010
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJC")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// 'E' is equivalent to 0b00100
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJE")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// 'I' is equivalent to 0b01000
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJI")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// '7' is equivalent to 0b11111
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJ7")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// '6' is equivalent to 0b11110
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJ6")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// '4' is equivalent to 0b11100
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJ4")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}
// 'Y' is equivalent to 0b11000
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJY")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "unused bits should be set to 0")
}

// 'Q' is equivalent to 0b10000, so there should be no error
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJQ")
assert.NotContains(t, err.Error(), "unused bits should be set to 0")

// Padding bytes are not allowed
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJLK===")
assert.Contains(t, err.Error(), "illegal base32 data")

// Invalid algorithm (low 3 bits of version byte are 7)
_, err = Decode(VersionByteMuxedAccount, "M47QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJUAAAAAAAAAAAACJUQ")
assert.Contains(t, err.Error(), "invalid version byte")

// Invalid checksum
_, err = Decode(VersionByteMuxedAccount, "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJUAAAAAAAAAAAACJUO")
assert.Contains(t, err.Error(), "invalid checksum")
}

func TestMalformed(t *testing.T) {
Expand Down
12 changes: 12 additions & 0 deletions strkey/encode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@ func TestEncode(t *testing.T) {
},
Expected: "GA3D5KRYM6CB7OWQ6TWYRR3Z4T7GNZLKERYNZGGA5SOAOPIFY6YQHES5",
},
{
Name: "MuxedAccount",
VersionByte: VersionByteMuxedAccount,
Payload: []byte{
0x3f, 0x0c, 0x34, 0xbf, 0x93, 0xad, 0x0d, 0x99,
0x71, 0xd0, 0x4c, 0xcc, 0x90, 0xf7, 0x05, 0x51,
0x1c, 0x83, 0x8a, 0xad, 0x97, 0x34, 0xa4, 0xa2,
0xfb, 0x0d, 0x7a, 0x03, 0xfc, 0x7f, 0xe8, 0x9a,
0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
},
Expected: "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJLK",
},
{
Name: "Seed",
VersionByte: VersionByteSeed,
Expand Down
7 changes: 4 additions & 3 deletions strkey/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const (
//VersionByteSeed is the version byte used for encoded stellar seed
VersionByteSeed = 18 << 3 // Base32-encodes to 'S...'

//VersionByteMuxedAccounts is the version byte used for encoded stellar multiplexed addresses
VersionByteMuxedAccount = 12 << 3 // Base32-encodes to 'M...'

//VersionByteHashTx is the version byte used for encoded stellar hashTx
//signer keys.
VersionByteHashTx = 19 << 3 // Base32-encodes to 'T...'
Expand Down Expand Up @@ -161,9 +164,7 @@ func Version(src string) (VersionByte, error) {
// is not one of the defined valid version byte constants.
func checkValidVersionByte(version VersionByte) error {
switch version {
// intentionally disallow M-strkeys (versionByteMuxedAccount)
// until SEP23 leaves the Draft status.
case VersionByteAccountID, VersionByteSeed, VersionByteHashTx, VersionByteHashX:
case VersionByteAccountID, VersionByteMuxedAccount, VersionByteSeed, VersionByteHashTx, VersionByteHashX:
return nil
default:
return ErrInvalidVersionByte
Expand Down
9 changes: 7 additions & 2 deletions txnbuild/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,16 @@ file. This project adheres to [Semantic Versioning](http://semver.org/).
### Breaking changes

* `AllowTrustOpAsset` was renamed to `AssetCode`, `{Must}NewAllowTrustAsset` was renamed to `{Must}NewAssetCodeFromString`.
* Some methods from the `Operation` interface (`BuildXDR()`,`FromXDR()` and `Validate()`) now take an additional `bool` parameter (`withMuxedAccounts`)
to indicate whether [SEP23](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0023.md) M-strkeys should be enabled.

### New features

Add support for Stellar Protocol 16 (CAP35): `Clawback` operations.

* Add support for Stellar Protocol 17 (CAP35): `Clawback`, `ClawbackClaimableBalance` and `SetTrustlineFlags` operations.
* Add opt-in support for [SEP23](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0023.md) M-strkeys for `MuxedAccount`s:
* Some methods from the `Operation` interface (`BuildXDR()`,`FromXDR()` and `Validate()`) now take an additional `bool` parameter (`withMuxedAccounts`)
* The parameters from `NewFeeBumpTransaction()` and `NewTransaction()` now include a new field (`EnableMuxedAccounts`) to enable M-strekeys.
* `TransactionFromXDR()` now allows passing a `TransactionFromXDROptionEnableMuxedAccounts` option, to enable M-strkey parsing.
## [v6.0.0](https://github.com/stellar/go/releases/tag/horizonclient-v6.0.0) - 2021-02-22

### Breaking changes
Expand Down
37 changes: 27 additions & 10 deletions txnbuild/account_merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@2opremio 2opremio Apr 14, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@2opremio 2opremio Apr 14, 2021

Choose a reason for hiding this comment

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

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

  1. I originally proposed what @bartekn is proposing now (you can go through the edit history of the issue body and see it).
  2. @tamirms suggested using an internal boolean (appended to each operation struct) at txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527 (comment)
  3. We discarded it at txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527 (comment) and went for the design as it is now.
  4. I proceed to implement the design for all the operations.
  5. @leighmcculloch proposed yet another design modification at txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527 (comment) which I discarded.
  6. @bartekn suggests my original design.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

var xdrOp xdr.MuxedAccount

err := xdrOp.SetAddress(am.Destination)
var err error
if withMuxedAccounts {
err = xdrOp.SetAddress(am.Destination)
} else {
err = xdrOp.SetEd25519Address(am.Destination)
}
if err != nil {
return xdr.Operation{}, errors.Wrap(err, "failed to set destination address")
}
Expand All @@ -27,29 +31,42 @@ func (am *AccountMerge) BuildXDR() (xdr.Operation, error) {
return xdr.Operation{}, errors.Wrap(err, "failed to build XDR OperationBody")
}
op := xdr.Operation{Body: body}
SetOpSourceAccount(&op, am.SourceAccount)
if withMuxedAccounts {
SetOpSourceMuxedAccount(&op, am.SourceAccount)
} else {
SetOpSourceAccount(&op, am.SourceAccount)
}
return op, nil
}

// FromXDR for AccountMerge initialises the txnbuild struct from the corresponding xdr Operation.
func (am *AccountMerge) FromXDR(xdrOp xdr.Operation) error {
func (am *AccountMerge) FromXDR(xdrOp xdr.Operation, withMuxedAccounts bool) error {
if xdrOp.Body.Type != xdr.OperationTypeAccountMerge {
return errors.New("error parsing account_merge operation from xdr")
}

am.SourceAccount = accountFromXDR(xdrOp.SourceAccount)
am.SourceAccount = accountFromXDR(xdrOp.SourceAccount, withMuxedAccounts)
if xdrOp.Body.Destination != nil {
aid := xdrOp.Body.Destination.ToAccountId()
am.Destination = aid.Address()
if withMuxedAccounts {
am.Destination = xdrOp.Body.Destination.Address()
} else {
aid := xdrOp.Body.Destination.ToAccountId()
am.Destination = aid.Address()
}
}

return nil
}

// Validate for AccountMerge validates the required struct fields. It returns an error if any of the fields are
// invalid. Otherwise, it returns nil.
func (am *AccountMerge) Validate() error {
_, err := xdr.AddressToAccountId(am.Destination)
func (am *AccountMerge) Validate(withMuxedAccounts bool) error {
var err error
if withMuxedAccounts {
_, err = xdr.AddressToAccountId(am.Destination)
} else {
_, err = xdr.AddressToMuxedAccount(am.Destination)
}
if err != nil {
return NewValidationError("Destination", err.Error())
}
Expand Down
16 changes: 15 additions & 1 deletion txnbuild/account_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,21 @@ func TestAccountMergeValidate(t *testing.T) {
},
)
if assert.Error(t, err) {
expected := "strkey is 4 bytes long; minimum valid length is 5"
expected := "invalid address"
assert.Contains(t, err.Error(), expected)
}
}

func TestAccountMergeRoundtrip(t *testing.T) {
accountMerge := AccountMerge{
SourceAccount: "GB7BDSZU2Y27LYNLALKKALB52WS2IZWYBDGY6EQBLEED3TJOCVMZRH7H",
Destination: "GB7BDSZU2Y27LYNLALKKALB52WS2IZWYBDGY6EQBLEED3TJOCVMZRH7H",
}
testOperationsMarshallingRoundtrip(t, []Operation{&accountMerge}, false)

accountMerge = AccountMerge{
SourceAccount: "MA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVAAAAAAAAAAAAAJLK",
Destination: "GB7BDSZU2Y27LYNLALKKALB52WS2IZWYBDGY6EQBLEED3TJOCVMZRH7H",
}
testOperationsMarshallingRoundtrip(t, []Operation{&accountMerge}, true)
}
Loading