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

feat: allow sending denoms with URL encoding #6847

Merged
merged 8 commits into from
Dec 12, 2023
Merged

feat: allow sending denoms with URL encoding #6847

merged 8 commits into from
Dec 12, 2023

Conversation

emidev98
Copy link
Contributor

@emidev98 emidev98 commented Nov 9, 2023

Enable querying token factory with URL encoded token denom, that way LCD can be queried without failing due the following error:

{
  "code": 12,
  "message": "Not Implemented",
  "details": []
}

This will not cause issues because denoms aren't allowed to have % which will no create any collisions

LCD Query;

image

CLI Query:

image

based on: terra-money/core#207

@mattverse
Copy link
Member

@emidev98 Thanks for the PR! Can you fix the lint please? 🙏

@emidev98
Copy link
Contributor Author

Hello @mattverse, for some reason the lint command does not work in my local. Could you run the linter please? Codewoners can push to the branch. Thanks!

@github-actions github-actions bot added the C:CLI label Nov 13, 2023
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Nov 28, 2023
@emidev98 emidev98 closed this Nov 28, 2023
@mattverse mattverse reopened this Nov 28, 2023
@mattverse
Copy link
Member

@emidev98 Happy to merge this once lint is fixed!

@github-actions github-actions bot removed the Stale label Nov 29, 2023
@ValarDragon ValarDragon self-requested a review November 29, 2023 19:06
@ValarDragon
Copy link
Member

Err we need to ensure we didn't introduce any serialization ambiguity here. We should be safe as long as:

  • URL escape only works with replacing input with a %
  • Chain denoms can't have % characters.

I feel uncomfortable relying on that first assumption in such a load bearing way though. Could we instead just do a find replace of %2F with /

@mattverse mattverse added the V:state/breaking State machine breaking PR label Nov 30, 2023
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Ah even better :) LGTM

@emidev98
Copy link
Contributor Author

Thanks @mattverse I have been very busy this days and didn't paid attention to some PRs.
I hope this helps you! 🥂

@mattverse
Copy link
Member

@ValarDragon Feel free to give secondary review for the change, planning to merge this by EOW!

@mattverse
Copy link
Member

mattverse commented Nov 30, 2023

@emidev98 Definitely does save alot of people's lives :) Thanks so much for the PR 😃

@mattverse mattverse merged commit bf63ca6 into osmosis-labs:main Dec 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants