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

Remove height from ConsensusState in ICS 007 implementation #7176

Closed
cwgoes opened this issue Aug 27, 2020 · 4 comments · Fixed by #7274
Closed

Remove height from ConsensusState in ICS 007 implementation #7176

cwgoes opened this issue Aug 27, 2020 · 4 comments · Fixed by #7274

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Aug 27, 2020

Storing the height in the value is redundant, because consensus states are always stored under a key which includes the height.

I think the only place we use this is here but it should be easy to pass the height through.

Ref https://github.com/cosmos/ics/issues/456

@cwgoes cwgoes added the x/ibc label Aug 27, 2020
@cwgoes cwgoes added this to the IBC 1.0 milestone Aug 27, 2020
@colin-axner
Copy link
Contributor

should we do this for solo machines as well in order to remove the GetHeight interface function? That would just mean moving the (latest) sequence to client state

@cwgoes
Copy link
Contributor Author

cwgoes commented Sep 4, 2020

Yes, that's fine I think.

@colin-axner
Copy link
Contributor

I think the only place we use this is here but it should be easy to pass the height through.

I'm honestly confused why this check is needed. It seems to me in all cases of its usage, the consensus state.Height checked was retrieved using the header.Height being checked against. The only time this check would fail is if for some reason we had been storing a different height in the consensus state than the one used for lookup. Maybe I am missing something?

@cwgoes
Copy link
Contributor Author

cwgoes commented Sep 9, 2020

It seems to me in all cases of its usage, the consensus state.Height checked was retrieved using the header.Height being checked against.

If that's true, we can remove it.

The only time this check would fail is if for some reason we had been storing a different height in the consensus state than the one used for lookup.

Are we still storing duplicate data here? Presumably this PR will fix that.

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

Successfully merging a pull request may close this issue.

3 participants