-
Notifications
You must be signed in to change notification settings - Fork 207
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
Fix Light Client validator set hash calculation #834
Conversation
Signed-off-by: Thane Thomson <connect@thanethomson.com>
This commit adds pagination to the `validators` method on the `Client` trait (BREAKING). Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
… string (like page numbers/per page counts) Signed-off-by: Thane Thomson <connect@thanethomson.com>
…from strings first Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
It's hard to know how to test this without doing (1) full integration testing or (2) mock testing with data captured from real nodes' RPC responses. Both would be useful, but both would still require a significant amount of work. Depending on how urgent this solution is, we could possibly do these in follow-up PRs? (1) would be particularly useful as a job that regularly runs in CI (e.g. on a daily basis). |
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
========================================
- Coverage 29.1% 28.8% -0.3%
========================================
Files 194 196 +2
Lines 10766 10865 +99
Branches 4443 4474 +31
========================================
- Hits 3139 3137 -2
- Misses 4653 4752 +99
- Partials 2974 2976 +2
Continue to review full report at Codecov.
|
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.
Looks great! Left some minor suggestions and a couple questions.
rpc/src/endpoint/validators.rs
Outdated
@@ -29,12 +66,28 @@ impl crate::SimpleRequest for Request {} | |||
|
|||
/// Validator responses | |||
#[derive(Clone, Debug, Deserialize, Serialize)] | |||
#[non_exhaustive] |
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 you explain why we need/what does this attribute brings?
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 in two minds about this right now.
On the one hand, I like the fact that it reduces the number of ways in which to construct a struct (this reduces the number of ways in which we can break downstream projects' code). If we restrict construction to the constructor, it also then, for certain structs, allows for parameter validation and additional guarantees that the resulting struct is most likely "correct".
On the other hand, we have some pretty massive structs where this doesn't work so well, where they have 9+ public fields. So the constructor-based approach becomes a little unwieldy. Usually I'd then use some builder pattern to construct such structs, but this will result in a significant amount more code (where I'm trying to avoid bloat, since if there's less code it's quicker to change).
I suppose, erring on the side of rapid development and less code (which is probably a good goal for the short- to medium-term), it's probably a better idea to go with the approach of direct struct construction?
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.
a)
On the one hand, I like the fact that it reduces the number of ways in which to construct a struct (this reduces the number of ways in which we can break downstream projects' code). If we restrict construction to the constructor, it also then, for certain structs, allows for parameter validation and additional guarantees that the resulting struct is most likely "correct".
Agreed, I think this is the right approach for structs that are exposed publicly and which we expect our users to instantiate themselves manually. But, even if possible, it's not really the case here as we expect users to use the higher-level methods rather than use perform
and construct the requests directly.
b)
On the other hand, we have some pretty massive structs where this doesn't work so well, where they have 9+ public fields. So the constructor-based approach becomes a little unwieldy. Usually I'd then use some builder pattern to construct such structs, but this will result in a significant amount more code (where I'm trying to avoid bloat, since if there's less code it's quicker to change).
If we ever want to use the builder pattern everywhere, this may help: https://lib.rs/crates/derive_builder :)
c)
I suppose, erring on the side of rapid development and less code (which is probably a good goal for the short- to medium-term), it's probably a better idea to go with the approach of direct struct construction?
Seems fine to me for now, given the reasoning I point a) above.
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.
derive_builder
looks amazing. We should definitely investigate using that approach for all the internal Tendermint domain types!
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
* Remove unused file Signed-off-by: Thane Thomson <connect@thanethomson.com> * Refactor validators RPC endpoint interface This commit adds pagination to the `validators` method on the `Client` trait (BREAKING). Signed-off-by: Thane Thomson <connect@thanethomson.com> * Ensure "total" response field is a string Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add serializer for optional types that need to be converted to/from a string (like page numbers/per page counts) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Refactor to ensure page numbers and per-page values are converted to/from strings first Signed-off-by: Thane Thomson <connect@thanethomson.com> * Convert tcp:// scheme to http:// for RPC addresses Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add Light Client support for RPC URLs instead of net::Address Signed-off-by: Thane Thomson <connect@thanethomson.com> * Revert 14ad69f for now Signed-off-by: Thane Thomson <connect@thanethomson.com> * Revert f0c26f7 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add CHANGELOG Signed-off-by: Thane Thomson <connect@thanethomson.com> * Convert infallible TryFroms to Froms Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove all non_exhaustive struct attributes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Attempt to fix broken wasm_bindgen dependency Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fixate syn dependency for light-client-js Signed-off-by: Thane Thomson <connect@thanethomson.com>
Closes #831