Memo-based Pooled Account Support

[JakeUrban]

resolves #727

Changes

SEP-10

• Adds the memo request parameters to the GET WEB_AUTH_ENDPOINT request
• Updates the account parameter to also accept muxed accounts (M...)
• Adds a memo to the challenge transaction returned from GET WEB_AUTH_ENDPOINT
• Updates the decoded JWT's sub value format to include the memo or be the muxed account used as the account parameter

SEP-6

• Clarifies GET /deposit's memo and memo_type request parameters strictly as the memo on the transaction submitted by the anchor to deliver on-chain funds
  • Previously, there was confusion around whether the memo parameters could also be used to identify users of a shared Stellar account, see #955
• Clarifies that the memo and memo_type request parameters of the GET /withdraw endpoint should only be used when the anchor does not support SEP-10 authentication
• Mandates anchors restrict the transactions returned from GET /transaction(s) endpoints to only be those created by requests containing the same sub value as the request for transactions

SEP-24

The changes described here are identical to those made for SEP-6, with one exception:

• Deprecates the memo and memo_type request parameters of the GET /withdraw endpoint, since SEP-10 is required

SEP-12

The changes made for this protocol are more nuanced because it is used by both SEP-6 and SEP-31. In SEP-6, authenticated sessions are scoped to a single user's (or customer's) transactions. In SEP-31, authenticated sessions are scoped to a Sending Anchor's transactions and customers.

This means that a client interacting with a SEP-12 implementation may be a Wallet acting on behalf of a single user, in which case the memo may be present in the SEP-10 JWT's sub value and therefore the memo and memo_type request parameters are no longer necessary, but for the sake of avoiding more changes to existing implementations should still be specified.

It also means that a client interactive with a SEP-12 implementation may be a SEP-31 Sending Anchor initiating the transfer of funds between two users, in which case the SEP-10 JWT shouldn't have memo or muxed account in the sub value. Instead, the SEP-31 anchor should use the memo request parameter to identify users.

SEP-12 implementors therefore must support both types of identifying users unless the protocol is only used for SEP-31 transactions.

• Removes text suggesting SEP-10 JWT's can be specified in URL's
  • There is no use case for this
• Deprecates the use of the account request parameter in all applicable endpoints
• Deprecates the use of the memo_type request parameter -- memos should always be of type id
• For all endpoints, the memo request parameter must match the memo portion of the JWT's sub value if present.
  • This ensure the anchor can use a single memo to identify the user in association with the Stellar account
• For all endpoints, the memo request parameter should not be accepted if the JWT's sub value is a muxed account.
  • This ensure the anchor does not use both a memo and muxed account to identify the same user.

[leighmcculloch]

👏🏻 I really like that this change was across multiple SEPs in a single PR. The broad nature of this change is easier realized applying it holistically. I reviewed SEP-6, 10, and 12. I did not review SEP-24 thoroughly given its similarity but I'll review it on my final pass. I left suggestions (💡) that I don't feel strongly about, questions (❓) that may result in changes but also may not, and concerns (❗) that I'd like to discuss further.

[leighmcculloch]

❗ I don't think we should say that rules apply to some SEPs and not others. SEP-12 needs to stand on its own with a single set of rules that can be applied uniformly without conditions on the SEP it is being used with, otherwise I think it is confusing to implement. I think we can tweak the language below to get the same result without presenting it as SEP-31 gets an exception to a rule.

Suggested change

	Note that this scenario is not applicable to [SEP-31](sep-0031.md)'s use of this protocol. This is because the Stellar account used to authenticate in this case identifies the [Sending Anchor](sep-0031.md#abstract) itself, not a particular customer, and therefore the scope of the authenticated session should include all customers registered and transactions initiated by the Sending Anchor.
	To facilitate this, SEP-31 Sending Anchors should use the `memo` and `memo_type` request parameters described for the endpoints below to represent unique customers instead of using SEP-10 memos.
	SEP-12 implementations should expect SEP-10 memos, if present in the decoded JWT, to match the memo request parameters and should return an error response if they do not.
	SEP-12 implementations should expect memo claims in the SEP-10 JWT to match the memo request parameters and should return an error response if they do not. If the JWT does not contain memo claims then the memo request parameters may contain any value. This behavior allows shared account owners, such as [SEP-31 sending anchors](sep-0031.md#abstract), to submit user information about their users using the memos assigned to their users.

Thoughts?

---

Note that this section appears to answer my earlier question of what it means if no memo is provided: authentication with a parent account should provide access to all data of sub-accounts identified by memos. It may be worth calling this out explicitly in SEP-10, or at least specifying in SEP-10 that it is undefined behavior and clients cannot rely on the outcome in SEPs that use SEP-10.

[JakeUrban]

authentication with a parent account should provide access to all data of sub-accounts identified by memos.

I have concerns about this approach, both from a security and application-logic perspective.

If a client application authenticates with an account that was previously used with memos, the session should be treated as disjoint from other sessions using memos with the same account.

This ensures the service never exposes transaction or KYC information that was not explicitly requested by the client using the composite primary key of (account, memo, memo_type).

This simplified application logic as well. Services can always use the three claims to retrieve user and transaction records instead of sometimes using memos and sometimes not.

# .get() either returns the value or None

def create_user(jwt):
    User.objects.create(account=jwt.get("account"), memo=jwt.get("memo"), memo_type=jwt.get("memo_type"))

def get_user(jwt):
    User.objects.get(account=jwt.get("account"), memo=jwt.get("memo"), memo_type=jwt.get("memo_type"))

[JakeUrban]

I agree with the text in the suggested change though, so I'll commit it but leave this thread unresolved.

[leighmcculloch]

I have concerns about this approach, both from a security and application-logic perspective.

If a client application authenticates with an account that was previously used with memos, the session should be treated as disjoint from other sessions using memos with the same account.

This is my preference and I share the same concern, but it sounds like the way it works with SEP-31 is that it isn't disjoint. I think we should endeavor to have our SEPs use SEP-10's scoping in the same way. How the scoping occurs is really out of scope of SEP-10 itself, but some consistency would have value.

Maybe we say in SEP-10 that it is not recommended, but then SEP-31/12 does it anyway?

[JakeUrban]

I'm not sure I understand how SEP-31's use of SEP-10 does not make sessions using the same account disjoint. In SEP-31's use of SEP-10, Sending Anchors will never request memos to be in their challenge transactions and downstream JWTs, so the Sending Anchor's session will always be disjoint from transactions and KYC data created in sessions authenticated by JWTs including memos. Let me know if you think I'm missing something.

[yuriescl]

If I understood correctly, these are the issues being discussed regarding the addition of memo and memo_type to SEP-10:

1. JWT sub claim requires uniqueness, and the account pubkey alone would not provide that
2. SEP-12 memo and memo_type parameters already refer to shared accounts, not to transaction memos

Sometimes I feel that SEPs need some kind of versioning and dependency setup. The SEPs have been through changes and they depend on each other, it can be difficult to always be backward compatible and want changes at the same time.
The memo changes would affect SEP-6, SEP-10, SEP-12, SEP-24 and (maybe?) SEP-31. There has to be some kind of dependency and versioning to allow Anchors and libraries to upgrade smoothly.
Even a basic system like this would be very useful:

• Let's say the changes are applied and we version the SEPs like this: SEP-10 v3, SEP-24 v2 and SEP-12 v1
• SEP-24 v2 would require SEP-10 v3 and be incompatible with SEP-10 v2
• SEP-12 v1 would require SEP-10 v2 and be incompatible with SEP-10 v3
• Etc.

Polaris and other SEP-dependant software could then be compatible to a set of versions, instead of trying to have all versions compatible with all SEP versions, which restricts space for non-breaking real-time improvement.

About the numbered points above, this is what I suggest:

1. Add a version claim to the JWT indicating the SEP-10 version. Any version prior to the memo addition will not contain the version claim and this means the sub claim will contain only the pubkey. Any version starting from the memo addition can have the memo value added to the sub claim (formats are to be discussed - maybe sub: {"account": "G...","memo": "SOME_MEMO","memo_type":"SOME_MEMO_TYPE"}? - I don't see a problem with that since everything could be in JSON format).
2. With a version system it's simple to avoid conflicts.

SEPs have API-like behavior. I've yet to see a complex API system without versioning work well.

[leighmcculloch]

@yuriescl Thanks for the commenting on versioning. There is definitely room for improvement and it's a long time coming I think. Let's continue discussion about that in #1046.

[leighmcculloch]

I think this looks good. I've left a few suggestions (💡), but they are only suggestions.

[accordeiro]

This looks good to me. There are a few details that I'd like to get ironed out before fully approving (see my comments below), but I think overall this is the right direction and set of changes.

[leighmcculloch]

I reviewed the changes since my last review when I approved, and I re-reviewed all the changes to SEP-10.

I've left one suggestion (💡) and one word change (🔠) but both are minor details.

This is great work and I think we've landed in the best place we can given the legacy constraints.

[accordeiro]

Looks great from my perspective, thank you @JakeUrban!

[leighmcculloch]

🎉