-
Notifications
You must be signed in to change notification settings - Fork 47
feat: F3 e2e lifecycle #1469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: F3 e2e lifecycle #1469
Conversation
fbaa095 to
b34142d
Compare
b34142d to
31feb85
Compare
91db005 to
cbce51c
Compare
39e59d6 to
bfdc6f7
Compare
93a1066 to
1747f78
Compare
…t and execute logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| /// | ||
| /// This method should only be called from consensus code path which | ||
| /// contains the lightclient verifier. No additional validation is | ||
| /// performed here as it's expected to be done by the verifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation in F3 Light Client update_state
High Severity
The update_state function in state.rs has no validation logic—it unconditionally replaces light_client_state and returns Ok(()). However, multiple tests expect it to reject invalid updates with USR_ILLEGAL_ARGUMENT: test_update_state_non_advancing_height expects rejection when height doesn't advance, test_instance_id_skip_rejected expects rejection when instance_id skips values, and test_empty_epochs_rejected also expects an error. These tests will fail because the validation they expect does not exist in the implementation.
Additional Locations (2)
| assert!(result.is_err()); | ||
| let err = result.unwrap_err(); | ||
| assert_eq!(err.exit_code(), ExitCode::USR_ILLEGAL_ARGUMENT); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name doesn't match test behavior
Low Severity
The test test_empty_epochs_rejected claims to test rejection of "empty finalized_epochs" per its comment on line 399, but it creates a state with Some(10) for latest_finalized_height rather than None. This makes it identical to test_update_state_non_advancing_height instead of testing the distinct case of a missing/empty finalized height. If the intent was to test rejection of None, the test should use create_test_state(1, None, ...).
Closes #1441 and #1442
Note
Cursor Bugbot is generating a summary for commit 8857277. Configure here.