-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
imp: make it easier to use custom verifiers for Tendermint clients #1168
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1168 +/- ##
==========================================
- Coverage 63.79% 63.72% -0.08%
==========================================
Files 219 218 -1
Lines 21389 21394 +5
==========================================
- Hits 13646 13633 -13
- Misses 7743 7761 +18 ☔ View full report in Codecov by Sentry. |
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.
Left some comments.
.changelog/unreleased/breaking-changes/1168-discard-TmVerifier.md
Outdated
Show resolved
Hide resolved
.changelog/unreleased/breaking-changes/1168-discard-TmVerifier.md
Outdated
Show resolved
Hide resolved
@@ -38,7 +42,7 @@ where | |||
ctx, | |||
client_id, | |||
client_message, | |||
&DefaultVerifier, | |||
&ProdVerifier::default(), |
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 am a bit confused about how I would reuse this interface for a custom verifier. After I implement Verifier
trait for my custom logic, how do I pass it here instead of ProdVerifier::default()
?
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.
See if this works. b8244a5
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.
Am I correct to conclude that someone has to create a wrapper struct:
struct CustomClientState(ClientState);
and delegate all calls except this one? If I am correct, can we add a generic to the current ClientState
?
struct ClientState<V: Verifier + Default> { ... }
type DefaultClientState = ClientState<ProdVerifier>;
type SovClientState = ClientState<SovVerifier>;
I am just confirming my understanding. If you already plan for this in future PRs, go ahead with this current merge.
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.
Am I correct to conclude that someone has to create a wrapper struct
That's right
Can we add a generic to the current ClientState
Our plan was to do this, but adding a generic introduces complications that eventually lead to refactoring some of the APIs as well. Aside from that, we didn't want to introduce complexity to normal users.
We can consider the generic or even writing macros in the future if there be a serious need.
Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com>
Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com>
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.
LGTM ✨ Thanks !
…1168) * imp: simplify introducing custom verifier object * chore: add changelog * Update .changelog/unreleased/breaking-changes/1168-discard-TmVerifier.md Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com> * Update ibc-clients/ics07-tendermint/src/client_state/validation.rs Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com> * Update .changelog/unreleased/breaking-changes/1168-discard-TmVerifier.md Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com> * Update ibc-clients/ics07-tendermint/src/client_state/validation.rs Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com> * Update ibc-clients/ics07-tendermint/src/client_state/validation.rs Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems> Signed-off-by: Sean Chen <seanchen11235@gmail.com> * docs: nudge toward implementing a newtype wrapper * nit --------- Signed-off-by: Sean Chen <seanchen11235@gmail.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com> Co-authored-by: Rano | Ranadeep <ranadeep@informal.systems>
Relevant context: #1166 (comment)
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.