feat: implement intoHeight trait#700
Conversation
|
@oblique i can switch out existing height assignments with trait based if you'd like. Anywhere specific to change? |
zvolin
left a comment
zvolin
left a comment
There was a problem hiding this comment.
Hey, thanks for the contribution. We were discussing this topic today and weren't sure which way to proceed with, so please give us some time to brainstorm that.
As to where to use it, I'd say in all celestia-rpc, celestia-grpc (and celestia-client when it's merged), and also on the lumina_node::Node` methods wherever height is required as an argument
types/src/lib.rs
Outdated
| mod extended_header; | ||
| pub mod fraud_proof; | ||
| pub mod hash; | ||
| pub mod height; |
There was a problem hiding this comment.
let's make the mod private and re-export the trait down below
types/src/height.rs
Outdated
| @@ -0,0 +1,47 @@ | |||
| use tendermint::block::Height; | |||
|
|
|||
| pub trait IntoHeight { | |||
There was a problem hiding this comment.
I think since this can be fallible, we should rename it to TryIntoHeight and try_into_height respectively
|
|
||
| // Error type for conversion failures | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum HeightConversionError { |
There was a problem hiding this comment.
We should also disallow 0 as height and have respective error
types/src/height.rs
Outdated
| // Error type for conversion failures | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum HeightConversionError { | ||
| NegativeValue, |
There was a problem hiding this comment.
| NegativeValue, | |
| NegativeHeight, |
types/src/height.rs
Outdated
| } | ||
|
|
||
| // Error type for conversion failures | ||
| #[derive(Debug, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
can you use thiserror for creating the error type? that's what we do for all other error types
2 participants
Fixes #693