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

Clarify the Height and ChainId types (ref: "epochs") #14

Open
2 of 5 tasks
adizere opened this issue Nov 18, 2020 · 2 comments
Open
2 of 5 tasks

Clarify the Height and ChainId types (ref: "epochs") #14

adizere opened this issue Nov 18, 2020 · 2 comments

Comments

@adizere
Copy link
Contributor

adizere commented Nov 18, 2020

Crate

ibc

Summary

The Height type and the ChainId types are closely related: the version_number field in a height of a chain denotes the revision of that chain. The revision used to be called "epoch," then renamed to "version", but it seems that the name of this field will stabilize to "revision".

There's two things to improve/clarify on our side:

  1. Our ChainId implementation is a bit messy and uses both the names of "version" and "epoch".

https://github.com/informalsystems/ibc-rs/blob/45ae7b86844c906570f7b16792ef4806ed45ab54/modules/src/ics24_host/identifier.rs#L17-L25

  1. The field of type Height might soon evolve so we probably need to rename version_number into revision_number.

https://github.com/informalsystems/ibc-rs/blob/b641f794cff8e5f21c308bd419fe4c29f028fda6/modules/src/ics02_client/height.rs#L9-L12

Dependencies:

Problem Definition

Proposal


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@thanethomson
Copy link

It'd make things a lot easier just to name this epoch everywhere. It has a pretty clear meaning and is quite distinct as a variable name.

Right now (from what I've managed to find so far) it's referred to as:

  1. revision_number
  2. version
  3. counter

depending on where it's being referenced.

@adizere
Copy link
Contributor Author

adizere commented Sep 29, 2021

I think using "epoch" as a field name in the Height type is not really an option, it would be more appropriate to align with the specs and the .proto and have the Height type consist of fields revision_number and revision_height. We did this alignment in this PR I think.

I'm not convinced using epoch in the ChainId type is a good idea. Does the term epoch originate from tendermint-go?

I think this issue refers to the problem that we use the notion of ChainId::epoch side-by-side with the notion of version and also the notion of revision_number. It seems like we could entirely eliminate both epoch and version and stick to revision_number. What do we lose then?

@hu55a1n1 hu55a1n1 transferred this issue from informalsystems/hermes Sep 29, 2022
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

No branches or pull requests

2 participants