[LEDGER] rm unnecessary mock from txmgr/validator #1094
Conversation
In the validator, we already create a mock for TxMgr interface but use the mock created at txmgr package. Hence, we make the validator to use the mock that present locally. Further, the validator does not need to create mock for all APIs in the TxMgr interface as it only uses NewTxSimulator(). Hence, we define a local interface. Signed-off-by: senthil <cendhu@gmail.com>
| func NewStatebasedValidator( | ||
| txmgr txmgr.TxMgr, | ||
| txSimProvider txSimulatorProvider, |
There was a problem hiding this comment.
Exported function receiving an unexported interface. Please don't do this.
There was a problem hiding this comment.
Sure. I would like to understand the reasoning so that it would stick in my mind.
My thought process was that valimpl package is not the one that implements this interface. It is just a consumer of the interface. Hence, an unexported interface was defined to implement polymorphism (to restrict API access). As this interface is to be used only by the valimpl package, I defined an unexported interface. If you can pinpoint the problem with the approach, I am sure that it will stick hard in my mind. I agree that it looks awkward to have an unexported interface but an exported function. However, when I consider the complete context, it makes sense to me. If there is specific reasoning for not employing this approach. I would like to understand and change my approach accordingly.
I just tried to export the interface. It then resulted in the import cycle. To fix that, we need to do some package restructuring as mentioned here -- https://stackoverflow.com/questions/50986170/how-to-avoid-import-cycles-in-mock-generation
Is package restructuring justifiable given that we would soon remove this dependency by moving the post-order tx processing to lockbasedtxmgr package FAB-17758? One could ask why we are not doing FAB-17759 first. As it requires a larger refactoring, we would want to do that towards the end once we reduce the folder depth and the number of packages in the ledger. The goal if this CR is to remove the TxMgr interface so that we don't need to have two packages such as txmgr and lockbasedtxmgr.
There was a problem hiding this comment.
Sure. I would like to understand the reasoning so that it would stick in my mind.
An exported API is public and expected to be consumed outside of the package. What this code does is say that my API is exported and you can pass me an argument that satisfies a contract - but I'm not really going to tell you what the contract is. This is inconsistent.
If a caller is supposed to satisfy an interface, the interface should be exposed so the caller can see how to satisfy it. It should also be possible for the caller to create a var of the type that you expect to hold its instance.
So, on public (exported) functions, the types of the arguments should also be public.
There was a problem hiding this comment.
What this code does is say that my API is exported and you can pass me an argument that satisfies a contract - but I'm not really going to tell you what the contract is. This is inconsistent.
Understood. Thanks.
@manish-sethi Should I move the post-order tx processing to lockbasedtxmgr to avoid the dependency? Once we do that, we can merge this PR as well as #1095 which would enable us to merge txmgr and lockbasedtxmgr packages.
There was a problem hiding this comment.
Given that we would have only one post-order tx, i.e., config block, we can in fact directly pass a simulator instead of TxMgr to create a simulator. When we move the post-order processing to lockbasedtxmgr, we can either retain the simulator or do a refactoring. Wdyt Manish?
There was a problem hiding this comment.
Based on the offline discussion with @manish-sethi we decided to remove the side-effect first before removing the TxMgr interface. Hence, I am marking this PR and #1095 as Draft.
Type of change
Description
In the serializability validator, though we create a local mock for the TxMgr interface, we use the mock created at
txmgrpackage. Hence, we make the validator to use the local mock and remove the one in thetxmgrpackage.Further, the validator does not need to create mock for all APIs in the
TxMgrinterface as it only usesNewTxSimulator(). Hence, we define a local interface to reduce the #LOC.Additional details
We pass
LockBasedTxMgrthat implementsTxMgrto the validator so that the validator can use it to get a tx simulator to execute the post-order transaction. As this is a side-effect, we need to run the post-order tx execution within the lockbasedtxmgr package and only use validator to perform the serializability check. Note that channel config tx is the only post-order tx execution. When we remove the side-effect, we can remove txSimulatorProvider interface introduced by this PR -- FAB-17758Related issues
FAB-17759