keep-network
Consistent approach for local chain implementations and chain types
We have a neat pattern we agreed on early in pkg/sortition where we define the local chain implementation in pkg/internal/local. This way, _test.go is only for actual tests and we can unit-test the local chain implementation as well.
Unfortunately, this pattern can cause import cycles. Suppose we implement tbtc.Chain in pkg/tbtc/internal/local.go, then the structure declared in this internal package must refer to tbtc chain types used by tbtc.Chain. On the other hand, the unit tests living in tbtc package must create an instance of local.Chain. This is why this pattern is used only in pkg/sortition where sorition.Chain does not declare any chain types.
One way to overcome this problem is to define chain types in a separate package as proposed here. pkg/tbtc/types would contain all types defined by the chain interface of tbtc package. This would allow us to avoid import cycles. The same pattern is used in go-ethereum: core/types or beacon/types. There are some concerns about bending the rule and referring to a sub-package from an upper-level package though.
Another way to overcome this problem is to de-flatten the structure of our packages and put the coordinator and maintainer packages under pkg/tbtc, moving the existing pkg/tbtc code into pkg/tbtc/wallet. There seems to be a broad agreement we are not in favor of this option given we like the flat structure of packages.
Yet one option is to declare a separate tbtccore package but there are concerns this would be a common-like package evolving over time into a monster structure that is hard to control.
The discussion: https://github.com/keep-network/keep-core/pull/3650#discussion_r1242336911
As a part of the work on this issue, we should also cleanup how mutexes work in local chain implementations. We should have a single per-struct mutex instead of defining them per field. This approach clutters the code and does not provide any significant boost for the unit test execution time.
