test: improve observer module coverage

[skosito]

Description

Improves coverage for observer module from 63.9% to 92.47%. Lot of tests were missing, but hopefully this is decent coverage now. Some notes:

• some header and proofs tests are not added because of this issue Reenable missing AddTracker tests and mock header/proof functionality for AddTracker and AddBlockHeader #1875
• events.go and migrator.go added to ignore in codecov
• i didnt add tests for past migrations
• couple of todos in comment, please check before merge

Closes: #1743

Type of change

Tests

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

• Tested CCTX in localnet
• Tested in development environment
• Go unit tests
• Go integration tests
• Tested via GitHub Actions

Checklist:

• I have added unit tests that prove my fix feature works

[github-actions]

!!!WARNING!!!
nosec detected in the following files: x/observer/keeper/hooks_test.go, x/observer/types/parsers_test.go, x/observer/abci.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.08%. Comparing base (09f2018) to head (f502001).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1961      +/-   ##
===========================================
+ Coverage    58.46%   63.08%   +4.62%     
===========================================
  Files          234      233       -1     
  Lines        13392    13381      -11     
===========================================
+ Hits          7829     8442     +613     
+ Misses        5159     4540     -619     
+ Partials       404      399       -5     

Files	Coverage Δ
x/observer/abci.go	100.00% <ø> (+100.00%)	⬆️
x/observer/genesis.go	100.00% <ø> (+23.48%)	⬆️
x/observer/keeper/utils.go	92.77% <100.00%> (+30.12%)	⬆️
x/observer/types/ballot.go	100.00% <100.00%> (+4.91%)	⬆️
x/observer/types/params.go	96.36% <ø> (+54.69%)	⬆️

... and 33 files with indirect coverage changes

[kingpinXD]

Looks great , left some comments please have a look.

Not part of the PR but some thoughts I had while reviewing

• The validation logic for maturity blocks should probably check that the number is positive
• When we remove the observer for the observer set we should probably pause the outbound, or at least have a way to notify that the observer set is modified. When it gets modified it would mean that the observer can no longer sign inbounds but still need to be part of the TSS sign for outbounds, which is weird?

[skosito]

Looks great , left some comments please have a look.

Not part of the PR but some thoughts I had while reviewing

• The validation logic for maturity blocks should probably check that the number is positive
• When we remove the observer for the observer set we should probably pause the outbound, or at least have a way to notify that the observer set is modified. When it gets modified it would mean that the observer can no longer sign inbounds but still need to be part of the TSS sign for outbounds, which is weird?

@kingpinXD thanks for review, i addressed your comments, please recheck when you get a chance

regarding your suggestions, including this one #1961 (comment) do you mind opening issues for it, since you have more context?

[kingpinXD]

Looks great , left some comments please have a look.
Not part of the PR but some thoughts I had while reviewing

• The validation logic for maturity blocks should probably check that the number is positive
• When we remove the observer for the observer set we should probably pause the outbound, or at least have a way to notify that the observer set is modified. When it gets modified it would mean that the observer can no longer sign inbounds but still need to be part of the TSS sign for outbounds, which is weird?

@kingpinXD thanks for review, i addressed your comments, please recheck when you get a chance

regarding your suggestions, including this one #1961 (comment) do you mind opening issues for it, since you have more context?

Added the issues here dc1a85e

I don't think the second point I mentioned needs to be fixed , this part of the logic should handle that
https://github.com/zeta-chain/zeta-node/blob/c7bf3d81ba6af3ed051d5158d06a55230374b0eb/x/observer/abci.go#L34-L35

[kingpinXD]

Looks good , unresolved one of the comments , please have a look before merging