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

Versioning is not fully consistent with ICS-es #97

Closed
Shivani912 opened this issue Jun 10, 2020 · 4 comments
Closed

Versioning is not fully consistent with ICS-es #97

Shivani912 opened this issue Jun 10, 2020 · 4 comments
Labels
A: help-wanted Admin: extra attention is needed, good for seniors
Milestone

Comments

@Shivani912
Copy link
Contributor

Shivani912 commented Jun 10, 2020

Problem

  • Cosmos SDK does not allow "empty" version strings but ICS-03 and ICS-04 suggest using a default version of "" if needed.
  • A lot of places in code use Version string and require some validation on version -> String and versions -> Vec<String>. For now, this is handled by two functions which are part of Connection module in code and this is simply to get the work done fast but this needs to be discussed on how we want to do it moving forward.

Proposed Solution

  • I suggest we adhere to the ICS-es to allow empty version strings and raise an issue about this on the SDK side.
  • I think it will be helpful to have a concrete Version type with its own implementation. I remember @ancazamfir saying this was being discussed and that it's not clear yet.
@Shivani912 Shivani912 added A: help-wanted Admin: extra attention is needed, good for seniors TODO labels Jun 10, 2020
@adizere
Copy link
Member

adizere commented Jun 11, 2020

Which ADR-03 and ADR-04 are we talking about exactly? (They're not in this repo, right?)

@ancazamfir ancazamfir changed the title Versioning is not fully consistent with ADR Versioning is not fully consistent with ICS-es Jun 11, 2020
@ancazamfir
Copy link
Collaborator

ancazamfir commented Jun 11, 2020

For now let's leave it like it as is in SDK code and your PR. I will move this to a later milestone (the relayer delivery one)

@ancazamfir ancazamfir added this to the 0.12-12mo milestone Jun 11, 2020
@Shivani912
Copy link
Contributor Author

Which ADR-03 and ADR-04 are we talking about exactly? (They're not in this repo, right?)

My bad @adizere I was talking about the ICSes 03 and 04

@adizere adizere removed the TODO label Nov 5, 2020
@ancazamfir
Copy link
Collaborator

Final fixes with #388

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: help-wanted Admin: extra attention is needed, good for seniors
Projects
None yet
Development

No branches or pull requests

3 participants