test(stf): add unit tests for processConsolidationRequest#290
test(stf): add unit tests for processConsolidationRequest#290lodekeeper-z wants to merge 2 commits into
Conversation
Cover 9 test cases for Electra consolidation request processing: - Unknown source/target pubkeys (early return) - Valid switch to compounding (source == target with ETH1 creds) - Self-consolidation without ETH1 credentials (no-op) - Incorrect source address mismatch - Target without compounding credentials - Source already exited (exit_epoch != FAR_FUTURE) - Source not active long enough (SHARD_COMMITTEE_PERIOD) - Happy path: valid consolidation with exit epoch set and pending consolidation added 🤖 Generated with AI assistance
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a suite of unit tests for the Electra consolidation logic. By covering various edge cases and the happy path, it ensures the robustness of the validation logic and state updates, which previously lacked test coverage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
🤖 Generated with AI assistance
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive test suite for the processConsolidationRequest function, covering scenarios like unknown public keys, credential mismatches, and validator activation states. The review identifies opportunities to improve code clarity by replacing magic numbers with constants and reducing boilerplate through a shared test context. Additionally, several style guide violations regarding line length limits and comment formatting were noted for correction.
| var validators = try state.validators(); | ||
| var validator = try validators.get(index); | ||
| var wc: [32]u8 = [_]u8{0} ** 32; | ||
| wc[0] = 1; // ETH1_ADDRESS_WITHDRAWAL_PREFIX |
There was a problem hiding this comment.
To avoid magic numbers and improve code clarity, please use the named constant ETH1_ADDRESS_WITHDRAWAL_PREFIX from constants.zig instead of the literal 1.
wc[0] = @import("constants").ETH1_ADDRESS_WITHDRAWAL_PREFIX;
| var validators = try state.validators(); | ||
| var validator = try validators.get(index); | ||
| var wc: [32]u8 = [_]u8{0} ** 32; | ||
| wc[0] = 2; // COMPOUNDING_WITHDRAWAL_PREFIX |
There was a problem hiding this comment.
To avoid magic numbers and improve code clarity, please use the named constant COMPOUNDING_WITHDRAWAL_PREFIX from constants.zig instead of the literal 2.
wc[0] = @import("constants").COMPOUNDING_WITHDRAWAL_PREFIX;
| const allocator = testing.allocator; | ||
| var pool = try Node.Pool.init(allocator, 256 * 5); | ||
| defer pool.deinit(); | ||
|
|
||
| var test_state = try TestCachedBeaconState.init(allocator, &pool, 256); | ||
| defer test_state.deinit(); | ||
|
|
||
| var state = test_state.cached_state.state.castToFork(.electra); |
There was a problem hiding this comment.
There's a lot of boilerplate for test setup repeated across all 9 tests. To improve maintainability and reduce code duplication, consider extracting this logic into a TestContext struct with init and deinit methods.
For example:
const TestContext = struct {
allocator: std.mem.Allocator,
pool: Node.Pool,
test_state: TestCachedBeaconState,
state: *BeaconState(.electra),
fn init(comptime validator_count: usize) !TestContext {
const allocator = testing.allocator;
var pool = try Node.Pool.init(allocator, validator_count * 5);
errdefer pool.deinit();
var test_state = try TestCachedBeaconState.init(allocator, &pool, validator_count);
errdefer test_state.deinit();
return .{
.allocator = allocator,
.pool = pool,
.test_state = test_state,
.state = test_state.cached_state.state.castToFork(.electra),
};
}
fn deinit(self: *TestContext) void {
self.test_state.deinit();
self.pool.deinit();
}
};Then each test could be simplified to:
test "consolidation request - unknown source pubkey" {
var ctx = try TestContext.init(256);
defer ctx.deinit();
const target_pubkey = try getValidatorPubkey(ctx.state, 1);
// ... rest of the test
}| const source_address: [20]u8 = [_]u8{0xAA} ** 20; | ||
|
|
||
| const request = makeConsolidationRequest(unknown_pubkey, target_pubkey, source_address); | ||
| try processConsolidationRequest(.electra, test_state.config, test_state.cached_state.epoch_cache, state, &request); |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the style guide. Please wrap the arguments to processConsolidationRequest to adhere to the line length limit. This applies to other similar calls in the tests as well.
try processConsolidationRequest(
.electra,
test_state.config,
test_state.cached_state.epoch_cache,
state,
&request,
);
References
- Hard limit all line lengths, without exception, to at most 100 columns for a good typographic "measure". (link)
| const request = makeConsolidationRequest(unknown_pubkey, target_pubkey, source_address); | ||
| try processConsolidationRequest(.electra, test_state.config, test_state.cached_state.epoch_cache, state, &request); | ||
|
|
||
| // No-op: pending consolidations should remain empty |
There was a problem hiding this comment.
According to the style guide (line 304), comments that are sentences should start with a capital letter and end with a period. Please update this comment and others in this file to follow this rule.
// No-op: Pending consolidations should remain empty.
References
- Comments are sentences, with a space after the slash, with a capital letter and a full stop, or a colon if they relate to something that follows. Comments after the end of a line can be phrases, with no punctuation. (link)
| var validators = try state.validators(); | ||
| var validator = try validators.get(0); | ||
| const wc = try validator.getFieldRoot("withdrawal_credentials"); | ||
| try testing.expectEqual(@as(u8, 2), wc[0]); |
There was a problem hiding this comment.
To avoid magic numbers and improve code clarity, please use the named constant COMPOUNDING_WITHDRAWAL_PREFIX from constants.zig for the expected value.
try testing.expectEqual(@as(u8, @import("constants").COMPOUNDING_WITHDRAWAL_PREFIX), wc[0]);
1 participant
Summary
Add 9 unit tests for
processConsolidationRequest(Electra consolidation logic), which previously had zero test coverage despite being 160 lines of complex validation logic with 10+ early-return branches.Test Cases
Note: The happy-path test overrides
total_active_balance_incrementson the epoch cache because mainnet preset requires ~500k+ validators for the consolidation churn limit to exceedMIN_ACTIVATION_BALANCE(with only 256 test validators, the churn limit is 0).🤖 Generated with AI assistance