-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Refactor CLIContext to allow multi-chain verifiers #5136
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5136 +/- ##
==========================================
+ Coverage 54.89% 54.98% +0.08%
==========================================
Files 296 297 +1
Lines 18232 18224 -8
==========================================
+ Hits 10009 10020 +11
+ Misses 7486 7466 -20
- Partials 737 738 +1 |
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.
One comment which can be resolved by updating the comment and godoc.
when connecting to multiple chains. The connecting chain's `CLIContext` will have to have the correct | ||
chain ID and node URI or client set. To use a `CLIContext` with a verifier for another chain: | ||
|
||
```go | ||
// main or parent chain (chain as if you're running without IBC) | ||
mainCtx := context.NewCLIContext() | ||
|
||
// connecting IBC chain | ||
sideCtx := context.NewCLIContext(). | ||
WithChainID(sideChainID). | ||
WithNodeURI(sideChainNodeURI) // or .WithClient(...) | ||
|
||
sideCtx = sideCtx.WithVerifier( | ||
context.CreateVerifier(sideCtx, context.DefaultVerifierCacheSize), | ||
) | ||
``` | ||
|
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.
Not sure if this is necessary, the description on the PR suffices imho
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.
True, but how often do people read PRs 😄
testCases := []struct { | ||
name string | ||
ctx context.CLIContext | ||
expectErr bool |
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.
Can we have one case with expectErr = false
?
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.
Looking at the Tendermint verifier code, unfortunately, I think that will require a running chain. So I don't think it's possible :-/
A couple of Q's:
|
@jackzampolin to answer your questions, I believe this PR should generally be supported and merged into |
CLIContext
CreateVerifier
which anyCLIContext
may call{home}/{chainID}/.lite_verifier
Example:
Note: All contexts share the same home directory for a CLI binary (i.e. there is no
WithHomeDir
).closes: #5116
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry to the
Unreleased
section inCHANGELOG.md
Re-reviewed
Files changed
in the github PR explorerFor Admin Use: