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

[ICS20-2] Final cleanup for multi denom #6109

Closed
4 tasks done
Tracked by #5793
chatton opened this issue Apr 8, 2024 · 3 comments
Closed
4 tasks done
Tracked by #5793

[ICS20-2] Final cleanup for multi denom #6109

chatton opened this issue Apr 8, 2024 · 3 comments
Assignees

Comments

@chatton
Copy link
Contributor

chatton commented Apr 8, 2024

This issue contains misc. points that have come up and need to be addressed before this feature can be shipped.

  • GetEscrowAddress uses a hardcoded string with value ics20-1. Reason if this could be changed without breaking backwards compatibility. (See which test(s) failed when changing this constant)
  • Need to make decision about: adding a new MsgTransferV2 with tokens (and no token field) or use existing MsgTransfer , add the tokens field and deprecate (and later remove) field token.
  • Maybe for the ICS20V1 and ICS20V2 const strings we could have an enum ICS20Versions with two values V1 and V2.
  • Add validation for the Token trace in packet v2 ValidateBasic
@chatton chatton added this to the ICS20 v2 milestone Apr 8, 2024
@colin-axner
Copy link
Contributor

GetEscrowAddress uses a hardcoded string with value ics20-1. Reason if this could be changed without breaking backwards compatibility. (See which test(s) failed when changing this constant)

Beyond backwards compatibility concerns, there was also a lot of address collision concerns pre-launch. The existing constant was verified not to have address collisions, so I'd suggest we leave as is. I'm unsure how outdated that discussion is, but best to not fix something that isn't broken

@chatton chatton self-assigned this May 21, 2024
@chatton
Copy link
Contributor Author

chatton commented May 21, 2024

Addressing the listed bullets:

  1. As per @colin-axner's comment, we can leave as "ics20-1", I will add a comment to make this more explicit.
  2. We will stick with using MsgTransfer and add a Tokens array instead of a MsgTransferV2
  3. Still open for debate, but thinking more about it, just keeping two string constants is fine by me. "ics20-1" and "ics20-2"
  4. Token trace validation has already been included here.

@chatton
Copy link
Contributor Author

chatton commented May 21, 2024

closed by #6345

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

No branches or pull requests

3 participants