-
Notifications
You must be signed in to change notification settings - Fork 316
Description
tmlibs and common are core dependencies of Tendermint containing a lot of somewhat independent code. Aggregating this code does make a lot of sense for versioning Tendermint, but for other projects like this one creates an undesirable level of coupling that could be avoided. I would like to propose that we drop the dependency on both. This would make it possible to upgrade Tendermint and iavl independently without forcing an upgrade of one or the other, which is the issue I am running into. I would argue that the code dependencies that iavl has on tmlibs and common are trivial, and particularly in the latter case is not pulling its weight.
tmlibs is currently imported for a subset of the DB interface and NewMemDB. This is convenient but means we pull in an entire dependency of tmlibs which makes it hard to use independently with Tendermint. Instead we could replicate the portion of the DB interface we need (Get, Set, NewBatch, and Iterator). MemDB is pretty trivial and the implementation against this subset interface would be even easier. This also adds a significant advantage for extensibility and mocking int that NodeDB would only be asking for what it actually needs - which is not the entire DB interface (e.g. https://blog.chewxy.com/2018/03/18/golang-interfaces/). I think this combined with decoupling from Tendermint/tmlibs is probably a good enough reason to accept the small amount of duplication. Admittedly iavl does depend on the semantics of the DB interface, but I think we could make clear that you may want use tmlibs in your iavl-based application - you would also have the flexibility to use a previous version with this subset interface (I think - and if not it would easier to make an adapter).
common is used for HexBytes, RandStr, Fmt, and PanicSanity. I think this is one of those cases where being DRY is the wrong thing to do. These are all relatively low value methods and don't seem to me to justify the dependency. They could either be replicated or removed. I feel like HexBytes is a convention that should belong to parent projects, RandStr introduces non-determinism into the unit tests which is not ideal, and Fmt and PanicSanity add very little.