-
Notifications
You must be signed in to change notification settings - Fork 268
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
Tendermint tmlibs and common dependencies #46
Comments
I'm in strong agreement. Would you be open to make a PR to remove the dependencies? Only thing is that it should be made very clear that the expect dbs come from tmlibs/db, but it shouldn't be a problem to replicate the parts of the interface we need and the MemDB itself within this repo. |
The DB interfaces are stable and useful. We should also consider using more of it and decoupling it from tmlibs. The pain is partially due to us packaging many dependencies together. As packages (like db) mature I think we should move the out, and db is ripe. (it benefited by being developed in tandem with the SDK and Tendermint, so I'm not knocking tmlibs in any way). I'd extend Dave's blog post with useful entangled interface sets like our tmlibs/db or https://golang.org/pkg/crypto/ . They are most useful/ergonomic when entangled (for as long as they are well designed and consistent), and they don't suffer from the smells that he's talking about. He mentioned recursive interfaces but I think entangled interfaces are the more general exceptional case. And this is also due to the design constraints of Golang where isometric interfaces aren't identical. If they could be used interchangeably then this wouldn't be an issue. https://play.golang.org/p/mUBkkbQPz8_s I'm more concerned about us making this proposed change without fully considering the reasons/heuristics, than the proposed change itself. It's easy enough to pull the DB.Iterator method out and define a new Well designed interfaces like the ones in DB are hard earned... Once they've matured IMO we benefit by using them more, not less. I'm sure we'll find better examples too in the future. |
I'm also on a mission to replace all instances of pkg/errors in all of our codebases, starting with IAVL, to this system: tendermint/tmlibs#220 , so eventually that will have to spin out of tmlibs as well. That can wait though, we can use pkg/errors for now. |
I agree with this. Extracting from tmlilb substantially solves the issue. In terms of enhanced (de)composition of the db interface. As you say it's an annoyance of Go's type system that we can't do this as it. It seems like we could pull out I have regularly thought it would have been useful to implement an interception layer for IAVL just for the subset of
I think asking for a minimal interface is a good heuristic in go anyway, but to give some context. I can see how defining the minimal interface at the consumer (e.g. IAVL) could lead to fragmentation though - and there is an advantage to centralising the component interfaces (in a db lib). There's less going on but here is the kind of approach I have taken: https://github.com/hyperledger/burrow/blob/develop/account/state/state.go We could have the same interface as currently exists as a composition, but have get-set, get-set-sync, batch, iterator, etc as separate component interfaces. IAVL could compose the ones it needs. Could try and make this more concrete with a PR in IAVL as a prototype if it would help. |
To add to this now IAVL has a dependency on Tendermint - for tendermint/libs/common and tendermint/crypto/tmhash. This is quite painful for upgrades since I dep now wants me to have compatible tendermint and IAVL versions. It looks like this dependency is mostly for errors.Wrap and some straight-forward hash conventions that I feel could easily be copied. Is it really worth the dependency? |
Having had a play with this - I agree with @jaekwon that there isn't really a nice way to extract the I'm up for making a PR that removes the other more trivial Tendermint dependencies and we could see how that looks in the meantime? |
Spinning of this db stuff is a fairly large design choice, we have chosen to adopt the monorepo style for better utilization of developer time. This would mean breaking off from it. We were actually discussing moving IAVL to the Cosmos-SDK as well. I'm not opposed to this iff the DB stuff is very stable. |
Personally I would like to see IAVL remain a separate library, but it is the worst of all worlds to have it in a separate repo but have dependencies on something has a heavy as Tendermint, so I suppose if that is the way it goes it would make sense to roll it into the cosmos-sdk. It was a nightmare having half a dozen dependency libraries I agree, and particularly since in reality only Tendermint depended on them - they weren't mature and moved too quickly for that to make sense. So stability is one factor, but another one is if you genuinely have multiple independent things depending on a library. For the db libs you currently have at least: Tendermint, Burrow, IAVL, and the cosmos-sdk. As @jaekwon says above:
I'm inclined to agree. I don't think the DB is necessarily 'very stable' - everything that depends on it is in principle alpha software! However I do think it is relatively more stable and it has real dependencies (and could attract more as a key-value db abstraction layer if maintained separately from Tendermint which I think would be for the good). |
Probably would be good to see
Meanwhile, it looks like IAVL is taking on some more dependence on Tendermint for the general merkle proof stuff - not quite sure how to resolve that, and where the general merkle proof stuff should live, but if it's going to be in Tendermint and the IAVL needs it, then maybe IAVL should move to Tendermint too. Ideally, nothing needs to move, and the IAVL just needs to implement some interfaces and maybe duplicate a bit of code, but otherwise doesn't have dependencies on Tendermint at all. So hopefully we can work towards that. |
Update: the only thing that is being used from |
for the time being, we will keep tendermint as a dep because of |
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 andNewMemDB
. 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
, andIterator
).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 thatNodeDB
would only be asking for what it actually needs - which is not the entireDB
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 theDB
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
, andPanicSanity
. 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 likeHexBytes
is a convention that should belong to parent projects,RandStr
introduces non-determinism into the unit tests which is not ideal, andFmt
andPanicSanity
add very little.The text was updated successfully, but these errors were encountered: