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.

Sign up for GitHub

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

Jump to bottom

all: Implement CAP 27 and SEP 23 #2471

Merged
merged 16 commits into from
Apr 16, 2020
Merged

all: Implement CAP 27 and SEP 23 #2471

merged 16 commits into from
Apr 16, 2020

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Apr 8, 2020 •
edited
Loading

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.

Fixes #2365 #2467

What

Implement CAP 27 and SEP 23

This PR doesn't provide a full integration with CAP27 though:

  • The ingestion code will simply transform MuxedAccounts into AccountIds
    (by dropping the memo ID). This means that queries will only show AccountId G-strkeys.
  • The POST transaction endpoint will accept transactions using CAP27s
    XDR, so clients need to be adapted to that.

Why

Because the Stellar protocol is everchanging.

Known limitations

Query responses won't contain MuxedAccounts (M-prefixed strkeys) where expected, instead horizon will internally (during ingestion) transform MuxedAccounts into AccountIds (G-prefixed strkey) by dropping the the memo id. This may come as a surprise to end-users.

@cla-bot cla-bot bot added the cla: yes label Apr 8, 2020
@2opremio 2opremio force-pushed the 2467-cap27-update-xdr branch 3 times, most recently from 6a47722 to c429e16 Compare April 9, 2020 18:26
@2opremio 2opremio changed the title Implement CAP 27 Implement CAP 27 and SEP 23 Apr 9, 2020
@2opremio 2opremio force-pushed the 2467-cap27-update-xdr branch 4 times, most recently from 493a1b7 to 419cceb Compare April 10, 2020 21:05
@2opremio 2opremio changed the title Implement CAP 27 and SEP 23 all: Implement CAP 27 and SEP 23 Apr 10, 2020
@2opremio 2opremio force-pushed the 2467-cap27-update-xdr branch from 1dc404e to 8de9cc1 Compare April 10, 2020 21:41
@2opremio 2opremio marked this pull request as ready for review April 10, 2020 21:47
@2opremio 2opremio requested review from abuiles and tamirms April 10, 2020 21:47
@2opremio
Copy link
Contributor Author

2opremio commented Apr 10, 2020 •
edited
Loading

Guide to reviewers: I would mainly focus on the xdr (new muxed_account files) and strkey packages (there is a lot of noise due to the type changes).

@tamirms
Copy link
Contributor

tamirms commented Apr 13, 2020

@2opremio I checked out your branch and searched for all cases where func (m *MuxedAccount) Address() string was invoked.

In the following cases I think we should be converting the MuxedAccount to an AccountId before calling Address() because we want to be ingesting "G..." addresses instead of "M..." addresses:

I also found one case in txsub where we should be converting a MuxedAccount to an AccountID before calling Address():

In this case we do a db lookup by account address and since we don't store M...' addresses in the database, the db lookup will not return any rows if a M..." address is used as the lookup key.

--

This requirement from issue #2365 is not addressed by your PR:

To fix this, you should also update ReadChallengeTx() in https://github.com/stellar/go/blob/2467-cap27-update-xdr/txnbuild/transaction.go#L447

--

For all of these cases that you fixed can you also add tests?

2opremio
2opremio commented Apr 13, 2020
View reviewed changes
strkey/main.go Outdated Show resolved Hide resolved
2opremio
2opremio commented Apr 13, 2020
View reviewed changes
strkey/main.go Outdated Show resolved Hide resolved
2opremio added 4 commits April 13, 2020 18:42
@2opremio
Implement SEP23 (MuxedAccount strkeys) and adapt Horizon to CAP27
4692237 
In a Nutshell:

1. The ingestion code will simply transformed MuxedAccounts into AccountIds
   (by dropping the memo ID).
   This means that queries will only show AccountId G-strkeys.
2. The `POST transaction` endpoint will accept transactions using CAP27s
   XDR, so clients need to be adapted to that.
@2opremio
Adapt txnbuild to CAP27
f37a65b
@2opremio
Fix the rest of the tests
242a11a
@2opremio
Fix shadow problem detcted by go vet
7c6be72
@2opremio
Disallow muxed accounts in challenge transaction
d68fe75 
I couldn't add tests because Transaction.Build() doesn't
allow using muxed accounts as input
@2opremio 2opremio force-pushed the 2467-cap27-update-xdr branch 4 times, most recently from 8866097 to d038322 Compare April 13, 2020 21:57
@2opremio 2opremio force-pushed the 2467-cap27-update-xdr branch from 81f881f to 149fd9a Compare April 14, 2020 15:33
@2opremio 2opremio force-pushed the 2467-cap27-update-xdr branch from e5d6e72 to 5fbe8e6 Compare April 15, 2020 19:53
@2opremio
Copy link
Contributor Author

@tamirms I think I have addressed everything, PTAL

tamirms
tamirms reviewed Apr 16, 2020
View reviewed changes
build/util.go Outdated
}

if m == nil {
return errors.New("aid is nil in setAccountId")
Copy link
Contributor

Choose a reason for hiding this comment

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

return errors.New("aid is nil in setMuxedAccount")

tamirms
tamirms reviewed Apr 16, 2020
View reviewed changes
txnbuild/transaction.go
@@ -449,6 +449,18 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti
if err != nil {
return tx, clientAccountID, err
}

// Enforce no muxed accounts (at least until we understand their impact)
muxedAccountErr := errors.New("muxed accounts are not allowed in challenge transactions")
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a test which exercises this failure mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but I I don't think it makes sense, see d68fe75

I couldn't add tests because Transaction.Build() doesn't
allow using muxed accounts as input

tamirms
tamirms approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

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

@2opremio this looks great! I found 2 small issues. Once those are fixed we can merge

@2opremio 2opremio merged this pull request into protocol-13 Apr 16, 2020
@2opremio 2opremio deleted the 2467-cap27-update-xdr branch April 16, 2020 13:45
@tamirms
Copy link
Contributor

tamirms commented Apr 17, 2020

@2opremio can you reopen the pr and target it to the horizon 1.2.0 release branch. I meant to retarget your PR earlier but I forgot about it. Unfortunately, I cannot push your commit onto the release branch without going through a PR because it is write protected

@2opremio 2opremio restored the 2467-cap27-update-xdr branch April 17, 2020 08:54
@2opremio
Copy link
Contributor Author

I cannot reopen it, I will need to create a separate PR. I will do that.

@2opremio 2opremio mentioned this pull request Apr 17, 2020
Merged
balboah added a commit to balboah/go that referenced this pull request Sep 16, 2020
@balboah
strkey: bitshift with unsigned int for Go 1.12 compatibility
20a2a4a 
Error was "shift count type int, must be unsigned integer".

This issue got introduced in stellar#2471
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamirms tamirms tamirms approved these changes

@abuiles abuiles Awaiting requested review from abuiles

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@2opremio @tamirms