ABCI layer tests

[avalkov]

Description

Implemented test that executes side tx over two heights like the real side tx workflow.
We have configurable array of tests where we can add more txs to be executed.

[Copilot]

Pull request overview

This PR implements comprehensive ABCI layer tests that execute side transactions over two block heights, simulating the real side transaction workflow. The changes refactor existing test utilities to be more flexible and add a new full end-to-end ABCI test.

Changes:

• Refactored vote extension test utilities to be more modular and reusable
• Updated all existing ABCI tests to use the new helper function signatures
• Added a new full ABCI workflow test that tests the complete lifecycle (PrepareProposal → ProcessProposal → ExtendVote → VerifyVoteExtension → FinalizeBlock → Commit) over two heights

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
helper/call.go	Refactored getTxReceipt to accept an interface for the client parameter, improving testability and flexibility
app/vote_ext_utils_test.go	Refactored vote extension setup functions: renamed and split setupExtendedVoteInfoWithNonRp into setupEmptyExtendedVoteInfo and createSignatureForVoteExtension for better modularity
app/abci_test.go	Updated all test functions to use the new buildExtensionCommits signature with an optional voteInfo parameter; moved helper functions to the top of the file; added duplicate function definitions
app/abci_full_test.go	New file with full ABCI workflow test demonstrating side tx execution over two heights with configurable test cases

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[avalkov]

@claude Please review this PR and highlight any potential issues.

[avalkov]

@claude Can you review this PR, focusing on safety and performance? Provide a summary as a comment highlighting any concerns related to these changes.

[kamuikatsurgi]

@claude Can you review this PR, focusing on safety and performance? Provide a summary as a comment highlighting any concerns related to these changes.

[adamdossa]

@avalkov @kamuikatsurgi FYI - I think we need PR #541 merged before the @claude comments above will work.

[avalkov]

@claude Can you review this PR, focusing on safety and performance? Provide a summary as a comment highlighting any concerns related to these changes.

[claude]

Claude finished @avalkov's task in 1m 15s —— View job

---

PR Review: ABCI Layer Tests - Safety and Performance Analysis

• Gather context and read changed files
• Analyze safety concerns
• Analyze performance concerns
• Provide summary with findings

---

Summary

This PR implements a comprehensive ABCI layer test that simulates the real side transaction workflow over two block heights. The changes are well-structured and improve test coverage for the consensus-critical ABCI layer. Overall, this is a solid addition to the test suite.

---

Safety Analysis

Positive Safety Improvements

1. Error handling improvement (app/vote_ext_utils_test.go:908-909): The createSignatureForVoteExtension function now properly checks the error from signing the non-RP vote extension with require.NoErrorf. This addresses the issue flagged by the Copilot review.

2. Interface-based testability (helper/call.go:1068-1072): Refactoring getTxReceipt to accept an interface instead of a concrete *ethclient.Client is a good design pattern that improves testability without changing production behavior.

3. Proper test isolation: The TestFullABCI test properly sets up mock callers and isolated test contexts, preventing test contamination.

Minor Concern

1. Signature verification removed (app/vote_ext_utils_test.go): The old setupExtendedVoteInfoWithNonRp function had a signature verification check (cmtPubKey.VerifySignature). While this was only printing an error (not failing), the new createSignatureForVoteExtension function removes this verification entirely. This is acceptable for tests since require.NoErrorf on the signing operation is sufficient, but worth noting.

---

Performance Analysis

Test Efficiency

1. Single-iteration loop concern (app/abci_full_test.go:71): The loop for i := 0; ; i++ with getTest returning nil for testIdx >= len(tests) is designed for extensibility, but currently executes only once. This is acceptable as the design allows easy addition of more test cases. Fix this →

2. App setup per test iteration (app/abci_full_test.go:72): The SetupAppWithABCICtx is called inside the loop, meaning a full app setup for each test case. This is appropriate for test isolation but could be expensive with many test cases. For the current single test case, this is not an issue.

3. Mock efficiency: Mocks are properly configured with mock.Anything and mock.AnythingOfType patterns, avoiding unnecessary strict matching overhead.

Memory Considerations

The test creates new testInfo structs with mock callers inside getTest on each call. For large test suites, consider moving mock setup outside the function, but for the current scope this is negligible.

---

Code Quality Observations

1. Good refactoring: Splitting setupExtendedVoteInfoWithNonRp into setupEmptyExtendedVoteInfo and createSignatureForVoteExtension improves modularity and reusability.

2. Consistent naming: The function names SetupAppWithABCICtx and SetupAppWithABCICtxAndValidators follow proper Go conventions.

3. Test coverage: The full ABCI workflow test covers PrepareProposal → ProcessProposal → ExtendVote → VerifyVoteExtension → FinalizeBlock → Commit which is comprehensive.

---

Recommendations

1. Documentation: Consider adding a brief comment at the top of app/abci_full_test.go explaining the purpose of the two-height side transaction workflow simulation.

2. Test naming: The test name "execute test" in t.Run("execute test", ...) at line 80 is generic. Consider using the test index or a more descriptive name.

3. Expand test cases: The configurable test array currently has one case. Adding more transaction types (e.g., milestone, clerk events) would increase coverage.

---

Verdict

No blocking safety or performance concerns. The PR is ready for merge from a technical standpoint. The changes improve test coverage for consensus-critical ABCI handlers and follow good testing patterns.

---

|

[sonarqubecloud]

❌ The last analysis has failed.

See analysis details on SonarQube Cloud

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[github-actions]

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[codecov]

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 42.3%. Comparing base (2cb4d7c) to head (08869a5).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
helper/call.go	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop    #539   +/-   ##
=======================================
  Coverage     42.2%   42.3%           
=======================================
  Files          172     172           
  Lines        17762   17769    +7     
=======================================
+ Hits          7506    7519   +13     
+ Misses        9177    9176    -1     
+ Partials      1079    1074    -5     

Files with missing lines	Coverage Δ
app/app.go	56.7% <100.0%> (ø)
x/bor/module.go	76.3% <100.0%> (-0.7%)	⬇️
x/checkpoint/keeper/keeper.go	58.6% <100.0%> (+0.5%)	⬆️
x/clerk/keeper/keeper.go	65.3% <100.0%> (+0.4%)	⬆️
x/clerk/keeper/side_msg_server.go	65.4% <100.0%> (ø)
x/clerk/module.go	85.7% <100.0%> (+5.7%)	⬆️
x/milestone/keeper/keeper.go	60.6% <100.0%> (+0.4%)	⬆️
x/stake/keeper/keeper.go	89.7% <100.0%> (+0.5%)	⬆️
x/stake/module.go	66.6% <100.0%> (-0.7%)	⬇️
x/topup/keeper/keeper.go	60.4% <100.0%> (+0.6%)	⬆️
... and 2 more

... and 5 files with indirect coverage changes

Files with missing lines	Coverage Δ
app/app.go	56.7% <100.0%> (ø)
x/bor/module.go	76.3% <100.0%> (-0.7%)	⬇️
x/checkpoint/keeper/keeper.go	58.6% <100.0%> (+0.5%)	⬆️
x/clerk/keeper/keeper.go	65.3% <100.0%> (+0.4%)	⬆️
x/clerk/keeper/side_msg_server.go	65.4% <100.0%> (ø)
x/clerk/module.go	85.7% <100.0%> (+5.7%)	⬆️
x/milestone/keeper/keeper.go	60.6% <100.0%> (+0.4%)	⬆️
x/stake/keeper/keeper.go	89.7% <100.0%> (+0.5%)	⬆️
x/stake/module.go	66.6% <100.0%> (-0.7%)	⬇️
x/topup/keeper/keeper.go	60.4% <100.0%> (+0.6%)	⬆️
... and 2 more

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
5.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
5.0% Duplication on New Code

See analysis details on SonarQube Cloud