test(stf): add unit tests for processWithdrawalRequest#294
test(stf): add unit tests for processWithdrawalRequest#294lodekeeper-z wants to merge 1 commit intoChainSafe:mainfrom
Conversation
Cover all major branches: - Unknown validator pubkey (no-op) - Missing execution credentials (no-op) - Mismatched source address (no-op) - Full exit request sets exit epoch - Already-exiting validator (no-op) - Partial withdrawal with compounding credentials - Partial withdrawal with ETH1 credentials (no-op) - Partial withdrawal capped at excess balance - Insufficient effective balance for partial (no-op) 🤖 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 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
|
![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 introduces a comprehensive test suite for the processWithdrawalRequest function, covering various scenarios such as unknown validator public keys, mismatched source addresses, and partial withdrawals with different credential types. The feedback identifies several violations of the project's style guide regarding variable naming, specifically the use of abbreviations like wc and non-descriptive names like validators2. Additionally, there is a recommendation to reduce code duplication across the new tests by implementing a shared test environment helper.
| var wc: [32]u8 = [_]u8{0} ** 32; | ||
| wc[0] = 1; // ETH1_ADDRESS_WITHDRAWAL_PREFIX | ||
| @memcpy(wc[12..32], &address); | ||
| try validator.setValue("withdrawal_credentials", &wc); |
There was a problem hiding this comment.
The variable wc is an abbreviation. The style guide (line 221) advises against abbreviating variable names for clarity. Please use the full name withdrawal_credentials.
var withdrawal_credentials: [32]u8 = [_]u8{0} ** 32;
withdrawal_credentials[0] = 1; // ETH1_ADDRESS_WITHDRAWAL_PREFIX
@memcpy(withdrawal_credentials[12..32], &address);
try validator.setValue("withdrawal_credentials", &withdrawal_credentials);
References
- Do not abbreviate variable names, unless the variable is a primitive integer type used as an argument to a sort function or matrix calculation. (link)
| var wc: [32]u8 = [_]u8{0} ** 32; | ||
| wc[0] = 2; // COMPOUNDING_WITHDRAWAL_PREFIX | ||
| @memcpy(wc[12..32], &address); | ||
| try validator.setValue("withdrawal_credentials", &wc); |
There was a problem hiding this comment.
The variable wc is an abbreviation. The style guide (line 221) advises against abbreviating variable names for clarity. Please use the full name withdrawal_credentials.
var withdrawal_credentials: [32]u8 = [_]u8{0} ** 32;
withdrawal_credentials[0] = 2; // COMPOUNDING_WITHDRAWAL_PREFIX
@memcpy(withdrawal_credentials[12..32], &address);
try validator.setValue("withdrawal_credentials", &withdrawal_credentials);
References
- Do not abbreviate variable names, unless the variable is a primitive integer type used as an argument to a sort function or matrix calculation. (link)
| 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.
This test setup boilerplate is repeated in all 9 new tests. This code duplication makes the test suite harder to maintain.
To improve this, you could introduce a TestEnvironment helper struct to encapsulate the setup and teardown logic. This would centralize the common code and make each test more concise and focused on its specific logic.
Here's an example of what that could look like:
const TestEnvironment = struct {
allocator: std.mem.Allocator,
pool: Node.Pool,
test_state: TestCachedBeaconState,
state: *types.electra.BeaconState.TreeView,
fn init(allocator: std.mem.Allocator) !TestEnvironment {
var pool = try Node.Pool.init(allocator, 256 * 5);
errdefer pool.deinit();
var test_state = try TestCachedBeaconState.init(allocator, &pool, 256);
errdefer test_state.deinit();
return TestEnvironment{
.allocator = allocator,
.pool = pool,
.test_state = test_state,
.state = test_state.cached_state.state.castToFork(.electra),
};
}
fn deinit(self: *TestEnvironment) void {
self.test_state.deinit();
self.pool.deinit();
}
};Each test could then be simplified to:
test "withdrawal request - unknown validator pubkey is no-op" {
var env = try TestEnvironment.init(testing.allocator);
defer env.deinit();
// ... test logic using env.state, env.test_state etc.
}References
- Code duplication should be avoided to improve maintainability and reduce the chance of errors when making changes.
| var validators2 = try state.validators(); | ||
| var validator2 = try validators2.get(0); | ||
| const exit_epoch = try validator2.get("exit_epoch"); |
There was a problem hiding this comment.
The variable names validators2 and validator2 are not very descriptive. The style guide (line 211: "Get the nouns and verbs just right") encourages using clear and meaningful names. To improve readability, consider renaming them to something like validators_after and validator_after to clarify that they represent the state after the function call.
var validators_after = try state.validators();
var validator_after = try validators_after.get(0);
const exit_epoch = try validator_after.get("exit_epoch");
References
- Great names are the essence of great code, they capture what a thing is or does, and provide a crisp, intuitive mental model. They show that you understand the domain. Take time to find the perfect name. (link)
| try testing.expectEqual(excess, try ppw.get("amount")); | ||
| } | ||
|
|
||
| test "withdrawal request - insufficient effective balance for partial is no-op" { |
There was a problem hiding this comment.
this group tests looks like could be used table driven the approach for simplify
2 participants
Summary
Add 9 unit tests for
processWithdrawalRequestcovering all major code paths:Tests added
Motivation
processWithdrawalRequesthad 0 test coverage. This is a complex Electra function with multiple early-return branches and the partial withdrawal queue logic.🤖 Generated with AI assistance