This better separate the responsabilities in the code and allow for
expanding the verifications easily. This will be later used for checking
an imported delegation.
Depends on D9420.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project
ninja all check-all
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- avalanche_peer_data_verify_proof
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 15518 Build 30956: Build Diff lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang Build 30955: arc lint + arc unit
Event Timeline
| src/avalanche/processor.cpp | ||
|---|---|---|
| 140 | Ok, this is getting somewhere, but see, if you kept the smart pointer, you'd have been able to return something nullable. Now you *HAVE TO* construct the PeerData object, which makes no sense when there is an error. You threw away the guarantee that the PeerData object is properly constructed with that design. But then you'll have another problem: what if this should actually be null, and it's not an error? Well, this is why the factory method should be about the processor, not the PeerData. Bonus point, you don't need to make PeerData part of the API. It's all win, but you got to ask yourself the right question before going down some path. It's not some hypothetical future change that might fubar things up, this is the code you just wrote telling you this. Read it. | |
| 238 | Can't you do a ctor style init? | |
| 242 | Not sure where this was introduced, but urg. This is pretty much the definition of temporal coupling: https://blog.ploeh.dk/2011/05/24/DesignSmellTemporalCoupling/ | |
| src/avalanche/processor.h | ||
| 205 | What i the purpose of this? Is this some independent entity that is of general use outside the Processor? If yes, then please add it to the description of the diff. If not, then please, stop. Ask yourself how you came to create some new publicly exposed entity that is of no real purpose and go fix that problem. It seems to me that the only purpose is initialization, which means go fix the initialization of this whole thing instead if patching layers on top of layers of wrong design. Don't you see it? You've now just enabled the mess to spread to the whole codebase instead of staying contained here. How is that a win? | |