Kolme cli: Find fork information #471
Conversation
Deploying kolme with
|
| Latest commit: |
e9fe810
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a1383ca1.kolme.pages.dev |
| Branch Preview URL: | https://kolme-cli-chain-version.kolme.pages.dev |
| } | ||
|
|
||
| #[derive(Deserialize, Copy, Clone, Debug)] | ||
| pub struct BlockHeight(pub u32); |
There was a problem hiding this comment.
I'm surprised you're defining your own type here instead of using the one in kolme itself, any motivation for that decision?
There was a problem hiding this comment.
I completely missed that. Will fix that!
| "Cannot compute middle since start_block is greater than end_block" | ||
| ); | ||
|
|
||
| let middle = block_height |
There was a problem hiding this comment.
NIT: middle seems like a confusing name here, since it seems like the delta to the middle, not the midpoint itself.
| } | ||
|
|
||
| /// Find an arbitrary block height with a particular chain version | ||
| async fn find_block_height(&self, chain_version: &str) -> Result<BlockResponse> { |
There was a problem hiding this comment.
Here's an idea... what about moving the logic for this entirely into the API server itself? It would require far less network traffic to pull off this search, likely speeding up the query significantly without any significant extra strain on the server (or, perhaps, even less strain on the server).
There was a problem hiding this comment.
Yeah, that would probably be more efficient than this client side binary search. I will shift this logic to server side, thanks!
| // Search in the upper half. | ||
| start_block = middle_block.succ()?; | ||
| } | ||
| _ => bail!("Impossible case"), |
There was a problem hiding this comment.
Hmm... strange case here. I'll understand better when I look at the version_compare module.
There was a problem hiding this comment.
The idea here is we are checking the three cases: Equal, Less than and greater than in the above cases. The remaining condition is impossible: Less than equal to, greater than equal to etc.
There was a problem hiding this comment.
My guess is that this isn't actually an impossible case, instead it may be that there's a version format the crate doesn't support.
There was a problem hiding this comment.
If I got it right, this logic seems to assume that versions in the chain follow SemVer and are ordered that way.
That said:
Maybe we could consider letting the user provide a custom comparison function, in case they use a different versioning scheme.
Also, regardless of how the comparison is done, I think it might make sense to always enforce that versions increase.
| }, | ||
| /// Send a transaction via an API server. | ||
| SendTx(SendTxOpt), | ||
| /// Fork height information |
There was a problem hiding this comment.
Maybe clarify what the point of this a bit more, e.g.:
| /// Fork height information | |
| /// Find the first block that has the given chain version |
| serde_json = { workspace = true } | ||
| tokio = { workspace = true } | ||
| tracing = { workspace = true } | ||
| version-compare = "0.2.0" |
There was a problem hiding this comment.
Oh, interesting. From a quick review of the crate, seems like a reasonable set of assumptions.
No description provided.