Tests: createInjector lack of test

[petrovska-petro]

Severity: can lead to unexpected behavior on lack of full coverage

Context: the test suite lacks a test covering at least two scenarios to ensure the correct/expected behaviour is executed:

1. Actual deployment of injector clone works as expected and parameters passed into
2. Verify actual event emitted is containing the right paramenters
3. is address return the expected for the injector?

All cases should be cover to have a rich coverage overall.

Recommendations: include a test covering the whole flow of injector (minimum: 1. & 2. cases described above)

In another note, it is generally advisable to add checks on the parameters passed in the function itself, i.e: currently in the flow initialize method does not have checks such as:

1. is owner and injectTokenAddress != than address(0). or other not desired values?. e.g: setting owner to address(0) will likely not be reversible. we acknoledge that for parameters such as keeperAddresses may be intentional to be empty to allow anyone calling the keeper methods
2. those parameters of type uint256 does not have neither a health-check? is expected minWaitPeriodSeconds to be 5 days, 10 months, 1 year? how can we ensure on-chain these parameters are within reasonable bounds when triggering initialize. It's advisable to be considerable with a reasonable bounds of time or amounts that operational we are expecting partners or 3rd user deploying injectors for the factory to use to avoid unexpected behaviour

Outcome overall improvement of coverage in the factory contract which currently is at zero.

Review stage

Balancer Maxis:

• Acknowledge
• Fixed

Onchainification Labs:

• Acknowledge correction/mitigation

[petrovska-petro]

Please provide feedback regarding the 0% coverage around the factory cc: @Tritium-VLK

[petrovska-petro]

@Tritium-VLK bumping this issue, not feedback yet

[Tritium-VLK]

@Hyferion Do you have a bit of time to write a test here? Feel free to ping me in telegram.
@petrovska-petro how important do you think it is. Is there some undesirable behavior you expect the test to reveal?

If we can't get tests done quick and easy, part of me would rather just deploy this somewhere and test the factory (and how it looks in etherscan, works on the UI, etc like that). Guess we could do that on testnet.

[petrovska-petro]

@Tritium-VLK we consider lack of coverage usually a poor practice while developing contracts that are going to hit production, regardless of how straightforward the logic may look like

But, given the pragmatic approach you're taking, we have 2 approaches:

1. if you feel comfortable with the testnet approach and verify at your end expected behaviour is correct, or
2. we could write the tests for this contract in foundry at handle the lack of coverage

Let us know which one its your preference

Note: if you end up deploying into testnet, feel free to share here the factory address and test-runs txs to also verify at our end

[petrovska-petro]

factory coveraged for the key function has being pushed in #45 capturing core logic, events and getter test ✅

issue is safe to close