all: Upgrade to protocol v12 #1776
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 24, 2019
tamirms
reviewed
Sep 25, 2019
| if err != nil { | ||
| return NewValidationError("DestMin", err.Error()) | ||
| } | ||
|
|
There was a problem hiding this comment.
do we need to validate pp.Path?
There was a problem hiding this comment.
Good point but I think it's outside of scope of this PR (path_payment_strict_send.go is a copy of path_payment.go adjusted for strict send). Tracking in #1786.
tamirms
reviewed
Sep 25, 2019
| @@ -741,10 +761,32 @@ func (is *Session) operationDetails() map[string]interface{} { | |||
| is.assetDetails(details, op.SendAsset, "source_") | |||
|
|
|||
| if c.Transaction().IsSuccessful() { | |||
| result := c.OperationResult().MustPathPaymentResult() | |||
| result := c.OperationResult().MustPathPaymentStrictReceiveResult() | |||
| details["source_amount"] = amount.String(result.SendAmount()) | |||
There was a problem hiding this comment.
if SendAmount() returns 0 if the transaction is not successful so maybe this line can be moved up and replace details["source_amount"] = amount.String(0)?
There was a problem hiding this comment.
Good point, changed!
tamirms
reviewed
Sep 25, 2019
|
|
||
| if c.Transaction().IsSuccessful() { | ||
| result := c.OperationResult().MustPathPaymentStrictSendResult() | ||
| details["amount"] = amount.String(result.DestAmount()) |
There was a problem hiding this comment.
if DestAmount() returns 0 if the transaction is not successful so maybe this line can be moved up and replace details["amount"] = amount.String(0)?
There was a problem hiding this comment.
Good point, changed!
tamirms
approved these changes
Sep 26, 2019
5 tasks
bartekn
added a commit
that referenced
this pull request
Oct 8, 2019
This commit updates Go monorepo to support Stellar Protocol v12. The new version changes some XDR types and adds a new `PathPaymentStrictSend` operation (see: CAP-24). Below are per package changes added to support v12. * XDR Go definitions have been regenerated using `xdrgen`. * Added `PathPaymentStrictSendResult.DestAmount()` helper method. Because this package is no longer supported the only changes are type name changes that allow the package to be compiled. No support for the new `PathPaymentStrictSend` operation. * Added a new `PathPaymentStrictSend` response to operation responses. * Added new error codes connected to `PathPaymentStrictSend` operation in `POST /transactions`. * Changed `/payments` to include `path_payment_strict_send` operations. * Updated `history_trades` table schema to allow amounts equal `0` (to support cases where `ClaimOfferAtom` contains `amountSold = 0` and `amountBought != 0`). * Updated `ingest` package to support ingesting operations, effects and trades that result from `path_payment_strict_send` operations. Support for `PathPaymentStrictSend` in auth endpoint. Added support `PathPaymentStrictSend` support. Check next section for more information. * Even though new operation has been implemented in Horizon's `ingest` package it has not been tested yet! New test scenarios are required but building them requires changes to `scc` (and `scc` requires changes to ruby SDK). * `PathPaymentStrictSend` has been added to `txnbuild` but there's an ongoing discussion whether we should release it now or after v12 upgrade. See: #1773. * Horizon schema change skips one number (`21` vs `20`) as `20` was used in Horizon 0.21.0 and it's not been merged into master yet.
7 tasks
bartekn
added a commit
that referenced
this pull request
Oct 10, 2019
In #1776 we changed code ingesting path payment operations to use helper methods (`DestAmount()` and `SendAmount()`) without checking if transaction is successful. The problem with that approach is that to call helper methods we need result structs fetched using `c.OperationResult()` however these may not exist if transaction was not successful or failed with `txFAILED` because for other error codes `results` slice is empty. This commit reverts the code in `operationDetails` to check if tx is successful before using helper methods. We also add a test to catch such case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
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.
Summary
This commit updates Go monorepo to support Stellar Protocol v12. The new version changes some XDR types and adds a new
PathPaymentStrictSendoperation (see: CAP-24).Summary of changes
Below are per package changes added to support v12.
xdrpackagexdrgen.PathPaymentStrictSendResult.DestAmount()helper method.buildpackageBecause this package is no longer supported the only changes are type name changes that allow the package to be compiled. No support for the new
PathPaymentStrictSendoperation.services/horizonandprotocols/horizonpackagesPathPaymentStrictSendresponse to operation responses.PathPaymentStrictSendoperation inPOST /transactions./paymentsto includepath_payment_strict_sendoperations.history_tradestable schema to allow amounts equal0(to support cases whereClaimOfferAtomcontainsamountSold = 0andamountBought != 0).ingestpackage to support ingesting operations, effects and trades that result frompath_payment_strict_sendoperations.services/compliancepackageSupport for
PathPaymentStrictSendin auth endpoint.txnbuildpackageAdded support
PathPaymentStrictSendsupport. Check next section for more information.Known limitations & issues
ingestpackage it has not been tested yet! New test scenarios are required but building them requires changes toscc(andsccrequires changes to ruby SDK).PathPaymentStrictSendhas been added totxnbuildbut there's an ongoing discussion whether we should release it now or after v12 upgrade. See: Update txnbuild to support stellar-core 12 (part 2 of 2) #1773.21vs20) as20was used in Horizon 0.21.0 and it's not been merged into master yet.