txnbuild: add muxed account & memo support to SEP-10 utility functions

[JakeUrban]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

What

resolves #3937

Adds support for muxed accounts to be specified as client account IDs and memos to be added to challenge transactions. This is a breaking change.

Why

SEP-10 was modified to support users of shared Stellar accounts. The two approaches to identifying users of a shared account, in the context of SEP-10, is by assigning each user a muxed account address or an integer memo.

Outstanding Questions

The Python SDK moved away from returning mutliple values from the BuildChallengeTx() and ReadChallengeTx() equivalent functions and instead introduced a ChallengeTransaction container object for all the relevant values. We could use this same approach in the Go SDK instead of adding an additional value to the existing set.

[tsachiherman]

Overall, I think that it looks good, but I do have few comments that would improve the code quality and readability.

[JakeUrban]

Thanks @tsachiherman,

Independently from your comments, what do you think about the approach described in the "Outstanding Questions" section of the description? Should we stick we adding another return value or use a ChallengeTransaction struct?

[leighmcculloch]

Thanks for expanding support for memos and muxeds in the SDK and also updating the webauth server too, this has been lacking for a while.

I left some comments inline.

[leighmcculloch]

The Python SDK moved away from returning mutliple values from the BuildChallengeTx() and ReadChallengeTx() equivalent functions and instead introduced a ChallengeTransaction container object for all the relevant values. We could use this same approach in the Go SDK instead of adding an additional value to the existing set.

I think a refactor like that should take place as an independent change either ahead of or after this change. I'd avoid making the change in the same pull request because it's really easy for the complexity of the diff to expand to the point where we miss subtle changes in the logic of these sensitive functions.

That aside, there are pros and cons.

I generally prefer a struct and I think it is more ergonomic. A side-effect of returning a struct is that each time we add a new field existing software will continue to work unchanged. This side-effect is usually a good thing because for most software consumers of values only need to be aware of a partial set of data contained in the value.

However, in the case of authentication it is usually important for the consumer of a value to be aware of the entire value. So for this code, while it is less ergonomic, I think the many return values actually play a beneficial role.

[JakeUrban]

Ok, thanks Leigh. We'll keep the use of multiple return values here.

[leighmcculloch]

One comment outstanding above:

• https://github.com/stellar/go/pull/4746/files#r1087283430

I think that there should be some tests added to the webauth service. There's a new parameter that's accepted, and new logic being added there.

For example:

[leighmcculloch]

I noticed another couple spots without coverage that seem important:

I think this is an area that was previously untested, but given we're adding some more complexity to whether an account is valid or not, I think it's probably worth adding tests for both cases.

[JakeUrban]

The 2nd check in first screenshot, where it errors if a memo and muxed account are provided, is tested. But I'll add tests for the 2 other checks.

[leighmcculloch]

👏🏻