-
Notifications
You must be signed in to change notification settings - Fork 499
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
services/horizon/internal: Allow path finding endpoints to take a list of assets instead of an account #1754
services/horizon/internal: Allow path finding endpoints to take a list of assets instead of an account #1754
Conversation
Codecov Report
@@ Coverage Diff @@
## release-horizon-v0.21.0 #1754 +/- ##
==========================================================
Coverage ? 45.33%
==========================================================
Files ? 392
Lines ? 26776
Branches ? 0
==========================================================
Hits ? 12139
Misses ? 13403
Partials ? 1234
Continue to review full report at Codecov.
|
…ccount Previously, the path finding endpoints used a destination or source account to represent a list of assets which constrain how each path orginates or terminates. Now, it is possible to provide a list of assets instead of an account.
7d99533
to
fecfdee
Compare
// "Code" must consist of printable ASCII characters (octets 0x21 through 0x7e). | ||
// The sequence \x introduces a hex escape sequence, e.g., \x00 to introduce a 0-valued byte. | ||
// Otherwise, \ escapes the next character, so \\ is required to introduce a backslash | ||
// https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0011.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to clarify this with core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, err | ||
} | ||
} else { | ||
parts := strings.Split(assetString, ":") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a minor thing but I think it's worth discussing at least: I think we should use character that is not escaped in URL query (ex. .
or -
). :
is changed to %3A
and it decreases readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good point but I'm not sure if we should deviate from sep 11. thoughts @ire-and-curses ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's annoying, but I'm in favour of sticking with the established convention from SEP 11.
if err != nil { | ||
return xdr.AccountId{}, errors.New("invalid address") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use AccountId.SetAddress
here. It contains strkey
and length check inside.
if len(query.SourceAssets) > handler.maxAssetsLength { | ||
p := problem.MakeInvalidFieldProblem( | ||
"source_assets", | ||
fmt.Errorf("list of assets exceeds maximum length of %v", handler.maxPathLength), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Errorf("list of assets exceeds maximum length of %v", handler.maxPathLength), | |
fmt.Errorf("list of assets exceeds maximum length of %d", maxAssetsForPathFinding), |
// FindPathsHandler is the http handler for the find payment paths endpoint | ||
type FindPathsHandler struct { | ||
staleThreshold uint | ||
maxPathLength uint | ||
checkHistoryIsStale bool | ||
maxAssetsLength int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we don't need this field if maxAssetsForPathFinding
is present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I should have deleted maxAssetsForPathFinding
. I started out using maxAssetsForPathFinding
but then I decided that it would be easier if that variable was configurable so I could add a test case where I provided too many assets in the param
log.WithFields(log.F{ | ||
"sourceAccount": sourceAccount, | ||
"numAssets": len(query.SourceAssets), | ||
}).Info("find paths request loaded assets for source account") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need to log this. We can get this data directly from stellar-core database.
pathFinder paths.Finder | ||
coreQ *core.Q | ||
maxPathLength uint | ||
maxAssetsLength int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above: do we need this? Why not use maxAssetsForPathFinding
?
The endpoint will not allow requests which provide both a `source_account` and a `source_assets` parameter. All requests must provide one or the other. | ||
The assets in `source_assets` are expected to be encoded using the following format: | ||
|
||
The native asset should be rendered as `"native"`. Issued assets should be rendered as `"Code:IssuerAccountID"`. `"Code"` must consist of printable ASCII characters (octets 0x21 through 0x7e). The sequence `\x` introduces a hex escape sequence, e.g., `\x00` to introduce a 0-valued byte. Otherwise, `\` escapes the next character, so `\\ `is required to introduce a backslash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we added docs to the other endpoint already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no docs for the other endpoint. should I add documentation now or wait until experimental ingestion is fully rolled out?
Co-Authored-By: Bartek Nowotarski <[email protected]>
Co-Authored-By: Bartek Nowotarski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I think we should remove all SEP-11 references. There's really no need to support low-level values (with 0
padding) and malformed asset codes (AssetAlphaNum12
with less than 5 chars).
I think that confusion comes from the fact that SEP-11 wants to be able to display low-level and malformed values (I think it shouldn't) but we definitely don't need to support this in Horizon for the same reason we display USD
instead of USD\x00
in asset fields in responses.
EDIT: it seems that a note explaining why SEP-11 supports malformed assets was removed, adding it back here: stellar/stellar-protocol#409.
fmt.Errorf("%s is not a valid asset", assetString), | ||
) | ||
} | ||
code, err := parseAssetCode(parts[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly we use parseAssetCode
to stick with SEP-11 however I don't think we should deal with low-level representation of asset code here (and in SEP-11 tbh, I'll create a PR). I don't think any app is using low-level, padded version of asset codes other than in XDR encoding/decoding. If they do, they shouldn't and we shouldn't accept low level representation in Horizon.
I think we should replace parseAssetCode
function with a simple ^[A-Za-z0-9]{1,12}$
regexp. xdr.Asset.SetCredit
adds padding so we don't need to do it here.
The endpoint will not allow requests which provide both a `source_account` and a `source_assets` parameter. All requests must provide one or the other. | ||
The assets in `source_assets` are expected to be encoded using the following format: | ||
|
||
The native asset should be rendered as `"native"`. Issued assets should be rendered as `"Code:IssuerAccountID"`. `"Code"` must consist of alphanumeric ASCII characters. Stellar disallows asset codes of type `credit_alphanum12` that have fewer than 5 bytes. To represent an asset code with fewer than 5 characters we must pad the asset code with trailing "\x00" characters. For example, the 12-byte asset code ABC is represented as "ABC\x00\x00". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To represent an asset code with fewer than 5 characters we must pad the asset code with trailing "\x00" characters. For example, the 12-byte asset code ABC is represented as "ABC\x00\x00".
stellar-core will not accept something like this thus such assets don't exist.
services/horizon/internal/web.go
Outdated
@@ -247,17 +247,20 @@ func (w *web) mustInstallActions(config Config, pathFinder paths.Finder) { | |||
// Transaction submission API | |||
r.Post("/transactions", TransactionCreateAction{}.Handle) | |||
|
|||
const maxAssetsForPathFinding = 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: let's move it to top.
Co-Authored-By: Bartek Nowotarski <[email protected]>
@bartekn can you take another look at the PR? the asset code validation issue should be fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks!
Brilliant :) |
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder 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
Previously, the path finding endpoints used a destination or source account to represent
a list of assets which constrain how each path begins or terminates.
Now, it is possible to provide a list of assets instead of an account.
Fixes #1678
Goal and scope
The motivation for this change is described in detail here #1678 . For full context please look at the issue.
When providing a source account, the in memory path finding implementation does extra validation such as checking that the source account has a sufficient balance when considering different payment paths. This validation was not present in the old path finding implementation.
It turns out that some consumers of the path finding endpoint would prefer if the endpoint did not cull payment paths based on properties of the source account. To accommodate this need we have included another request parameter consisting of a list of assets which can be used instead of a source account. When using an asset list instead of a source account, the path finding implementation will not perform any validation checks related to the source account.
In order to make the
/paths/strict-receive
and/paths/strict-send
endpoints symmetric, I have added a similar change to the/paths/strict-send
implementation.Summary of changes
source_account
parameter from/paths/strict-send
to make the endpoint more symmetric with/paths/strict-receive
destination_assets
parameter to/paths/strict-send
andsource_assets
parameter to/paths/strict-send
Known limitations & issues
We do not check for asset liabilities and trustlines limits. This should be implemented in a separate PR.
What shouldn't be reviewed
N / A