Complexity of Manager and Insufficient Test Coverage

Open oxnr opened this issue 2 years ago • 0 comments

Finding 007 - Complexity of Manager and Insufficient Test Coverage

ID 007

Finding Complexity of Manager and Insufficient Test Coverage

Severity 3 - Informational

Description The Manager type contains mixed usage of multiple channels and a mutex to synchronize various aspects of the Manager. This leads to greater complexity in debugging, upgrading, and general understanding of the implementation. Especially around the notion of what data is protected and synchronized.Important methods such as AggregationLoop, SyncLoop, and BlockStoreRetrieveLoop are rather complex with little documentation on their function and how the channels are used within them.In addition, there seems to be a lack of test coverage for such an important component of the Rollkit codebase.

Recommendation We see there is markdown documentation that explains the overall concepts and roles of each component and the general algorithm of the Manager and state management. However, we believe it would be prudent to improve the Godoc documentation of critical and public APIs of the Manager type. Specifically, outline the role and use of each channel and how Contexts come into play.In addition, consider refactoring some of the critical APIs to make them easier to understand and test.Finally, we recommend considerably increasing test coverage and edge case testing of the Manager type.

Code References https://github.com/rollkit/rollkit/blob/eccdd0f1793a5ac532011ef4d896de9e0d8bcb9d/block/manager.go

ID	007
Finding	Complexity of Manager and Insufficient Test Coverage
Severity	3 - Informational
Description	The Manager type contains mixed usage of multiple channels and a mutex to synchronize various aspects of the Manager. This leads to greater complexity in debugging, upgrading, and general understanding of the implementation. Especially around the notion of what data is protected and synchronized.Important methods such as AggregationLoop, SyncLoop, and BlockStoreRetrieveLoop are rather complex with little documentation on their function and how the channels are used within them.In addition, there seems to be a lack of test coverage for such an important component of the Rollkit codebase.
Recommendation	We see there is markdown documentation that explains the overall concepts and roles of each component and the general algorithm of the Manager and state management. However, we believe it would be prudent to improve the Godoc documentation of critical and public APIs of the Manager type. Specifically, outline the role and use of each channel and how Contexts come into play.In addition, consider refactoring some of the critical APIs to make them easier to understand and test.Finally, we recommend considerably increasing test coverage and edge case testing of the Manager type.
Code References	https://github.com/rollkit/rollkit/blob/eccdd0f1793a5ac532011ef4d896de9e0d8bcb9d/block/manager.go

Jan 17 '24 22:01 oxnr