Skip to content
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

SPIKE: Breakdown of code areas to investigate in first batch #12442

Closed
kantp opened this issue Jan 3, 2023 · 4 comments
Closed

SPIKE: Breakdown of code areas to investigate in first batch #12442

kantp opened this issue Jan 3, 2023 · 4 comments

Comments

@kantp
Copy link
Member

kantp commented Jan 3, 2023

The objective of this spike is to get a good enough understanding of the code and tests in the new portion of the codebase to make a plan for how to break down the following investigation of how good our test coverage is.

We will need to look at the diff between develop and compatible to find the major new features (mostly, zkApps). We will want to create a list of new components, where they are in the codebase, and how large they are. With that, we can then create individual tickets to look at the test coverage of each of those.

@Sventimir
Copy link
Member

Sventimir commented Jan 17, 2023

One of the places where ZK App commands are processed is src/lib/mina_base/zkapp_command.ml module. This library does not have any test modules, but zkapp_command.ml has some inline tests inside. Visually these tests are few and likely insufficient, although admittedly functions in this module seem to be quite basic, so it is possible that for some of them static type checking is enough. Overall though, I think we could try to expand the tests for this part of the code.

Also there are almost no comments and the code is far from self-explanatory, so improving on that would also be worthwhile.

@Sventimir
Copy link
Member

Module src/lib/transaction_logic/zkapp_command_logic.ml is completely without any tests and it seems to be a pretty significant part of the system. It contains a functor delivering a function to apply a zkapp command to any existing state (apparently). This function is in addition very long (~800 lines of code) and could probably benefit from being split into smaller operations. Some comments explaining how it works would also be nice to have.

@Sventimir
Copy link
Member

Sventimir commented Jan 18, 2023

In addition to that, unfortunately Zkapp_command_logic and Mina_transaction_logic throw a lot of exceptions, which is suspicious and makes things more difficult to verify. A refactoring aiming at improving the type safety of this code would also be in order.

@Sventimir
Copy link
Member

I created issues to write tests for the modules mentioned above: #12530 and #12531. Therefore I close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants