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

services/horizon: Add Muxed Account details to operation responses #3603

Merged

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented May 17, 2021

Part of #3591

It follows the design discussed at #3591

It abuses the operation details to encode source_account_muxed and source_account_muxed_id

TODO:

  • Add tests

@2opremio 2opremio requested a review from a team May 17, 2021 18:22
protocols/horizon/operations/main.go Outdated Show resolved Hide resolved
Limit string `json:"limit"`
Trustee string `json:"trustee"`
Trustor string `json:"trustor"`
TrustorMuxed string `json:"trustor_muxed,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think trustor can be muxed: https://github.com/stellar/stellar-core/blob/b9e10051eafa1125e8d238a47e5915dad30c2640/src/xdr/Stellar-transaction.x#L241

Not all accounts can be muxed. I would double check against the xdr definitions. MuxedAccount can be muxed, AccountID can not.

Copy link
Member

@ire-and-curses ire-and-curses May 17, 2021

Choose a reason for hiding this comment

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

Yes - I itemised these in this issue for reference @2opremio : #3490

Copy link
Member

Choose a reason for hiding this comment

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

One complication: M addresses are not fully interchangeable with G. If you grep for AccountID in Stellar-transaction.x, you'll see the places where AccountID was preserved. These are: CreateAccount, SetOptions, AllowTrust, and SponsoringFutureReserves, as well as various operation results. The parsing implementation should disallow such invalid address uses.

Copy link
Member

@leighmcculloch leighmcculloch May 17, 2021

Choose a reason for hiding this comment

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

I find it helpful to look at where we need to add support, which is everywhere MuxedAccount is used.

/workspaces/stellar-core $ git grep -p -n MuxedAccount -- src/xdr/
src/xdr/Stellar-transaction.x=7=namespace stellar
src/xdr/Stellar-transaction.x:11:union MuxedAccount switch (CryptoKeyType type)
src/xdr/Stellar-transaction.x=77=struct PaymentOp
src/xdr/Stellar-transaction.x:79:    MuxedAccount destination; // recipient of the payment
src/xdr/Stellar-transaction.x=95=struct PathPaymentStrictReceiveOp
src/xdr/Stellar-transaction.x:102:    MuxedAccount destination; // recipient of the payment
src/xdr/Stellar-transaction.x=120=struct PathPaymentStrictSendOp
src/xdr/Stellar-transaction.x:125:    MuxedAccount destination; // recipient of the payment
src/xdr/Stellar-transaction.x=376=struct ClawbackOp
src/xdr/Stellar-transaction.x:379:    MuxedAccount from;
src/xdr/Stellar-transaction.x=413=struct Operation
src/xdr/Stellar-transaction.x:418:    MuxedAccount* sourceAccount;
src/xdr/Stellar-transaction.x:439:        MuxedAccount destination;
src/xdr/Stellar-transaction.x=472=case ENVELOPE_TYPE_OP_ID:
src/xdr/Stellar-transaction.x:475:        MuxedAccount sourceAccount;
src/xdr/Stellar-transaction.x=550=struct Transaction
src/xdr/Stellar-transaction.x:553:    MuxedAccount sourceAccount;
src/xdr/Stellar-transaction.x=585=struct FeeBumpTransaction
src/xdr/Stellar-transaction.x:587:    MuxedAccount feeSource;

So, based on that, I see these places as the only places we should be expecting it:

  • FeeBumpTransaction feeSource
  • Transaction sourceAccount
  • Operation sourceAccount
  • PaymentOp destination
  • PaymentPaymentStrictReceiveOp destination
  • PathPaymentStrictSendOp destination
  • ClawbackOp from
  • AccountMergeOp (there's not actually a type for this in the XDR) destination

Copy link
Contributor Author

@2opremio 2opremio May 17, 2021

Choose a reason for hiding this comment

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

@leighmcculloch there are some derived fields in operations which are also muxed

Also, note that this PR is specific about operations

Copy link
Contributor Author

@2opremio 2opremio May 17, 2021

Choose a reason for hiding this comment

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

I don't think trustor can be muxed

Note that we use the operation source as trustor: https://github.com/stellar/go/pull/3603/files#diff-5f37fbbec67583bbaa94cbe645d8a4aa046505cee2c37be660818ef340645ab1L346-R355 and the source is muxed

My criteria to mark something as muxed or not was whether what we use when populating the operation details is muxed or not (see the changes in operations_processor.go)

@2opremio 2opremio force-pushed the 3591-add-muxed-details-to-operations branch 2 times, most recently from 27b64c4 to 9583ad4 Compare May 19, 2021 11:39
@2opremio 2opremio marked this pull request as ready for review May 19, 2021 11:39
@2opremio 2opremio force-pushed the 3591-add-muxed-details-to-operations branch from 9583ad4 to 9aaf030 Compare May 20, 2021 14:23
@2opremio 2opremio force-pushed the 3591-add-muxed-details-to-operations branch from 9aaf030 to 5131c4c Compare May 20, 2021 14:40
@2opremio 2opremio merged commit f289da6 into stellar:master May 20, 2021
@2opremio 2opremio deleted the 3591-add-muxed-details-to-operations branch May 20, 2021 16:19
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

5 participants