Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: remove GetHeight from ClientMessage interface #1285
chore: remove GetHeight from ClientMessage interface #1285
Changes from 3 commits
d72c9e1
9737a7e
b30380f
832ca2e
9116fd1
2066ab9
aca9870
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not emitting
UpdateClientEvent
on a successfulMsgUpdateClient
is problematic. I may be wrong though. Would be good to check with relayers what the best expected functionality is for dup updatesThe main odd thing is if you query for UpdateClient events, this message wouldn't come up. To me, it'd make more sense if the event was emitted, but indicated it was a duplicate update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to change this to return the height from the
ClientState
implementation on duplicate updates.However, I don't think we can provide information about duplicate updates in the events, that would only be possible if emitting from
07-tendermint
for example, but as we discussed before they should be emitted from02-client
in a somewhat standardised fashion.If we're in agreement, I can update this PR to return the height on duplicates and also make the changes to remove the
error
return value and panic instead.Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking if
consensus_heights
is empty, we should just return "" in the consensus heights attribute. Thoughts?In favor of removing
error
return 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated and removed the error return, and also returning the height on duplicate header updates from 07!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think we need a len(consensusHeights) > 0 check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was being checked in the
UpdateClient()
method of keeper.go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes. The only strange part is that this function assumes you pass in a non empty array of consensus heights
Unrelated, could we add
// deprecated
next to theconsensus height
attributeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we backport
consensusHeights
attribute to older releases? That way relayers could switch to it without waiting for all chains to upgrade to this versionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, it is kind of strange. I'll adapt to check
len(consensusHeights)
inside event emission func and remove the check from theUpdateClient
keeper method.Sounds like a good idea, but this would require a separate PR to main, correct?
Edit: I'll also add deprecated comment in-line 👍 Now that I'm looking at this once again,
AttributeKeyConsensusHeight
is used for create and upgrade events as a single height value, and should probably be maintained. In which case, I think the deprecation notice should only be here, solely for the update events and not intypes/events.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!