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

Feature Request: maxAssetsForPathFinding should be configurable #4037

Closed
marcinx opened this issue Oct 28, 2021 · 0 comments · Fixed by #4046
Closed

Feature Request: maxAssetsForPathFinding should be configurable #4037

marcinx opened this issue Oct 28, 2021 · 0 comments · Fixed by #4046
Assignees

Comments

@marcinx
Copy link
Contributor

marcinx commented Oct 28, 2021

What problem does your feature solve?

We recently added additional trustlines to our accounts and hit the hardcoded limit of 15 assets in pathfinding requests. This resulted in breaking things on our end.

What would you like to see?

Is it possible to make

const maxAssetsForPathFinding = 15
configurable by Horizon operators?

What alternatives are there?

We'd have to refactor a whole lot of our code base to iterate over pathfinding requests.

Other observations (bug reports)?

  1. The error messages in

    fmt.Errorf("list of assets exceeds maximum length of %d", handler.MaxPathLength),
    and
    fmt.Errorf("list of assets exceeds maximum length of %d", handler.MaxPathLength),
    use the wrong variable. It should be handler.MaxAssetsParamLength instead of handler.MaxPathLength

  2. It is possible to omit the above max asset validation check by submitting a source_account or destination_account instead of a list of assets. This is not a good substitute however as the pathfinder then behaves differently (and takes the balance of the source account into consideration in strict-receive requests). Also, the validation check for max assets is probably in place for a reason (e.g. vs cpu flooding?) so this could be a potential exploit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants