chore: move net2 dependency out of tendermint crate
#338
Conversation
| use tendermint::abci::{Code, Log, Path}; | ||
| use tendermint::block; | ||
| use tendermint::merkle::proof::Proof; | ||
| use tendermint::serializers; |
There was a problem hiding this comment.
Just out of curiosity: are all these import changes due to a different formatter? Is there a re reason why all the use crate:: ...s are replaced in that fashion?
There was a problem hiding this comment.
Personal touch really, optimised for editability and tracebility of dependencies. I usually one liner per top-level module in a crate.
There was a problem hiding this comment.
Ok for me. Ideally, these should be consistent in all our crates though.
| pub(crate) use custom::parse_non_empty_block_id; | ||
| pub(crate) use custom::parse_non_empty_hash; | ||
| pub use custom::parse_non_empty_block_id; | ||
| pub use custom::parse_non_empty_hash; |
There was a problem hiding this comment.
Do we have to publish these beyond the crate? Probably, yes as some might be used in the rpc crate now?
There was a problem hiding this comment.
Indeed, the rpc crate depends on a lot of these serialisers. Not happy with it. Also found the excessive use of pub(crate) is unnecessary as the top-level lib determines visibility beyond crate boundaries. Eager to hear how we can address this issue.
There was a problem hiding this comment.
If they are only used in the rpc lib, they could actually live there. Not sure yet.
There was a problem hiding this comment.
They are also used in tendermint::block.
There was a problem hiding this comment.
Maybe removing the net dependency outweighs this now public modules/methods (serializers).
I might have mentioned that somewhere else before but ideally, the serialzation code would simply be abstracted away in the sense that anyone using the tendermint crate can e.g. simply call block::deserialize (or deserialize_json and deserialize_binary) and the that could internally use a dedicated serialization type serialize::block (and impl Froms/Intos between both) which uses methods / helpers like the above. Anyone (like the rpc crate) could simply use the core types and wouldn't need to care about serialization specific stuff. Does that make sense? Probably worth another discussion and later another PR.
There was a problem hiding this comment.
For now: maybe the serializers module should include a comment that we don't guarantee any backwards compatibility on the methods in the module between any releases and consider them an internal implementation detail (use at you own risk alike).
There was a problem hiding this comment.
We need to lay out an explicit semantic versioning scheme and declare which modules we're covering and which we're not. Good to start tracking like this though thanks!
There was a problem hiding this comment.
Awesome. Thanks @xla! Left some comments (looks like s/rpc/crate/g changed some comments to sound funny, e.g. "request json crate").
But more relevant is that some of the changes seem to have messed with the integration tests:
thread 'rpc::abci_info' panicked at 'there is no reactor running, must be called from the context of Tokio runtime',
e.g.: rpc::abci_info
| @@ -0,0 +1,20 @@ | |||
| [package] | |||
| name = "tendermint-rpc" | |||
There was a problem hiding this comment.
This probably deserves a short readme (can be done in a followup PR).
| serde = { version = "1", features = [ "derive" ] } | ||
| serde_bytes = "0.11" | ||
| serde_json = "1" | ||
| tendermint = { version = "0.13.0", path = "../tendermint" } |
There was a problem hiding this comment.
I'm always sceptical of circular dependencies (even if it's just a dev-dependency). Are we confident that this does cannot have weird side-effects?
There was a problem hiding this comment.
I would not expect any, but truth be told this was the solution with the least amount of opinionated changes. Would like to get rid of this circle as well, but afraid this goes too far for this PR.
There was a problem hiding this comment.
I can't think of any problems either 🍀 🤞
On second thought it might be worthwhile to keep tokio in |
xla
dismissed
liamsi’s stale review
Major points addressed, curious to get another pass.
| @@ -2,6 +2,10 @@ | |||
| //! | |||
| //! Serializers and deserializers for a transparent developer experience. | |||
| //! | |||
| //! CAUTION: There are no guarantees for backwards compatibility, this module should be considered | |||
| //! an internal implementation detail which can vanish without further warning. Use at your own | |||
| //! risk. | |||
This is isolates the code paths which depend on network and http functionality by moving the rpc module into its own crate (tendermint-rpc). The expected outcome is that third-parties which depend on the tendermint crate are not forced to pull in unnecessary dependencies. Furthermore it will isolate any potential crates used to drive an rpc server implementation going forward.
Controversially this also removes the dependency ontokioas it was mostly used for async test execution, which was achieved by making extensive use offutures::executor::block_on- we've come full circle. Eager to hear opinions and alternatives to that.Update: now a dev-dependency only instead of the above.
Follow-up to #7
Preparation for #219
Closes #289
Wrote tests