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

Bug: correctly handle denoms with / in pcli tx lp #4766

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

cronokirby
Copy link
Contributor

The parsing looked for the first occurrence of the slash for

nX@nY/fee

but if X or Y contains a slash, this will be incorrect.

This fixes this somewhat, by looking for the last slash, and only considering that a fee if it ends with bps.

This will not correctly handle a denom containing both a slash and ending with bps, but beyond that we'll need to change the format probably.

Issue ticket number and link

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    client side change only, for pcli

The parsing looked for the first occurrence of the slash for

nX@nY/fee

but if X or Y contains a slash, this will be incorrect.

This fixes this somewhat, by looking for the last slash, and only
considering that a fee if it ends with bps.

This will not correctly handle a denom containing both a slash and
ending with bps, but beyond that we'll need to change the format
probably.
@cronokirby cronokirby added the C-bug Category: a bug label Jul 25, 2024
Copy link
Contributor

@aubrika aubrika left a comment

Choose a reason for hiding this comment

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

Seems good enough for now, we can address the denom incompatibility issue if/when it arises 🤷🏻‍♀️

@aubrika aubrika merged commit acbfe7f into main Jul 25, 2024
13 checks passed
@aubrika aubrika deleted the bug-pcli-parse-parts branch July 25, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: a bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants