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

Support stellar-core protocol 12 (CAP 24) #1737

Closed
7 of 13 tasks
ire-and-curses opened this issue Sep 10, 2019 · 8 comments
Closed
7 of 13 tasks

Support stellar-core protocol 12 (CAP 24) #1737

ire-and-curses opened this issue Sep 10, 2019 · 8 comments
Assignees
Labels
horizon txnbuild 2nd-generation transaction build library for Go SDK

Comments

@ire-and-curses
Copy link
Member

ire-and-curses commented Sep 10, 2019

The upcoming stellar-core protocol 12 release will add support for CAP 24 ("Make PathPayment Symmetrical").

Horizon
Currently, we return type: path_payment in \operations and \payments endpoints (doc). In Horizon 0.22:

Before v12 vote: (#1772)

  • Note deprecation of returned type in \operations and \payments source code
  • Update the XDR library in the monorepo with the v12 definition files from stellar-core
  • Update Horizon to map incoming new types to path_payment. Continue to accept old path_payment type
  • Update documentation for operation types - indicate that response may not match underlying operation, and mention both old and new operation names
  • Note deprecation in release notes. Schedule to start returning path_payment_strict_receive and path_payment_strict_send in type field in two releases time (Horizon 0.24)
  • Release

SDK
Before v12 vote: (#1771)

  • Add deprecation warning to txnbuild to indicate that the existing PathPayment operation will be renamed to PathPaymentStrictReceive in a few weeks' time
  • Add new operation PathPaymentStrictReceive to SDK, duplicating existing PathPayment operation. Programmers can begin moving client calls immediately
  • Raise corresponding build issues for other SDKs (including JS and Java)
  • Update Godoc and examples
  • Release

After v12 vote: (#1773)

  • Release new operation PathPaymentStrictSend to SDK, implementing new v12 operation
  • Update scc
@ire-and-curses ire-and-curses added horizon txnbuild 2nd-generation transaction build library for Go SDK labels Sep 10, 2019
@ire-and-curses
Copy link
Member Author

@bartekn Anything I missed?

@bartekn
Copy link
Contributor

bartekn commented Sep 11, 2019

We also need to update scc.

@tomquisel
Copy link
Contributor

@MonsieurNicolas does this look good to you?

@ire-and-curses
Copy link
Member Author

@jonjove said:

Starting in protocol 12 with CAP-0024, it will be possible for a ClaimOfferAtom to contain amountSold = 0 and amountBought != 0 (only for the new PathPaymentStrictSend). For context, it has already been possible since at least protocol 10 for amountSold = amountBought = 0 but it was guaranteed that if either was nonzero then both were nonzero. Will this change be breaking for you?

We'll check this during implementation.

@MonsieurNicolas
Copy link
Contributor

afaik the only gotcha are related to those new trade results (from jonjove above) and ensuring that the new path finding algorithm is indeed computing path based on the most pessimistic way of crossing offers (which is slightly different than the original path finding) - this should be tracked here as well.

For the rename: it looks to me that most of the changes fall under the consequence of Update the XDR library in the monorepo with the new definition files, with the exception of Add deprecation warnings to Horizon and txnbuild to indicate that the existing PATH_PAYMENT will be renamed to PathPaymentStrictReceiveOp in a few weeks' time?

Was that the approach taken when we changed ManageOfferOp to ManageSellOfferOp (CAP006)? Was there some good lessons learned then?
It may be simpler to not go through this "warning" phase and just have a breaking change that does the rename (as supporting old + new implies that you would diverge from the xdr spec).

@tomquisel
Copy link
Contributor

Thanks! I think the warning phase is something we need to do to keep users of the clients happy.

@bartekn bartekn added this to the Horizon 0.22.0 milestone Sep 16, 2019
@ire-and-curses
Copy link
Member Author

Yeah, to be clear, we update the XDR immediately, but the name of associated structs/methods in the SDK doesn't change for a while. This isn't a problem since they are binary compatible. We also add the new operation type to the SDK but leave the old one present. This gives callers time to prepare for the name change.

@bartekn
Copy link
Contributor

bartekn commented Sep 24, 2019

Added a draft PR updating entire monorepo to v12 XDR (including txnbuild, horizon and other packages relying on XDR definitions like compliance server or deprecated build): #1776.

I wanted to update XDR only but realized that adding a support for the new operation to existing packages and services is rather easy so added it in one PR. There are still some simple things so I believe this will be ready for review tomorrow.

There's a larger task of adding new Horizon test scenarios for path_payment_strict_send operation what I think will be done in a separate PR (depends on scc so hard to estimate how long it will take to update it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
horizon txnbuild 2nd-generation transaction build library for Go SDK
Projects
None yet
Development

No branches or pull requests

4 participants