test(core-state): increase coverage to 100%

[bertiespell]

Summary

Adding tests for the core-state package.

Some refactor work to support the tests.

I chose to reuse a Sandbox environment for each test, which sets up and injects every class needed. However, because of the way classes are tagged (and different classes need access to different tagged versions) this may create complications (see discussion below).

Checklist

• Documentation (if necessary)
• Tests (if necessary)
• Ready to be merged

I've left some TODOs in the code to highlight areas where I was unsure what the intended behaviour was.

Most of these issues occur in WalletRepository and TempWalletRepository - since the latter inherits from the former. Inheritance can be useful for small classes to extend some simple base functionality, but if the two classes are designed to behave differently or become complex - it can become quite difficult to maintain. One problem is that a method in the extended class can call into a method in the base class, which itself calls a method back in the extended class. This can make it harder to reason about the code, it has the potential to cause subtle bugs and makes the code more difficult to maintain. I note here too that there is a 3rd PoolWalletRepository which extends from this class and lives in a different package. It might be worth thinking about whether a shared common interface might be a better approach here?

I found that I lost a lot of time setting up tests because of issues with tagging. When two versions of a class (using the same identifier) are bound via tags - if this identifier is then consumed by a third class which does not specify a tag - which tagged class should it fall back to? Is this an intended consequence, or are we using this "fallback" as a feature? I'd suggest that code should be explicit in its intentions, rather than implicit. In the ServiceProvider, the WalletRepository and TempWalletRepository classes are bound to the same identifier via tags. However, BlockState consumes an untagged version of this identifier - what should this default to - or will it only ever exist in an environment where one tagged version could be injected? (If this is true, it would mean we cannot share the same initialised Sandbox for all tests). Despite initialising the tests by binging the two classes as they are done in the ServiceProvider - inversify fails with: Ambiguous match found for serviceIdentifier (suggesting that this would happen at runtime). In order to get the tests running, I’ve added a tag to the WalletRepository consumed by BlockState for now, but this isn’t the correct solution (since presumably we want BlockState to be able to work with both tagged versions), do let me know if anyone has suggestions on how to fix this, or whether I'm misunderstanding something...

Finally, two tests are intentionally left failing in StateBuilder.test.ts as the code implementation here needs review.