Conversation
|
Claude finished @iamquang95's task in 2m 58s —— View job Code Review
SummaryThis PR ports Charon's MEV relay test suite into the Pluto CLI, adding Findings[High]
[Medium] Timeout in
[Medium]
[Low]
[Low]
[Low] Hardcoded signature in
Parity Matrix
TestsUnit tests added:
No integration tests (requiring live relay) — acceptable given the external dependency. Clippy/fmt could not be verified (build tools not available in this environment). Open Questions
|
- Follow the `test validator` approach (enum + impl block) - Simplify error handling for timeouts
- Test is already cancelled by caller appropriately
- Workaround for false positive
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
I took the liberty of pushing some changes related to CancellationToken, timeouts and the like, that could cause trouble. I did not add them as comments due to the extent of the required changes.
There are a couple of things that require attention, like reusing existing error variants, mismatched features against v1.7.1 and missing tests.
| #[arg( | ||
| long = "x-timeout-ms", | ||
| default_value = "1000", | ||
| help = "X-Timeout-Ms header flag for each request in milliseconds, used by MEVs to compute maximum delay for reply." | ||
| )] | ||
| pub x_timeout_ms: u32, |
There was a problem hiding this comment.
This flag does not exist in v1.7.1 (reference Charon impl). Please remove it.
crates/cli/src/commands/test/mev.rs
Outdated
| r = apply_basic_auth(client.get(&clean_url), creds) | ||
| .header("X-Timeout-Ms", x_timeout_ms.to_string()) | ||
| .header( | ||
| "Date-Milliseconds", | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis() | ||
| .to_string(), | ||
| ) |
There was a problem hiding this comment.
No need for any of these headers (v 1.7.1)
| let body = resp | ||
| .bytes() | ||
| .await | ||
| .map_err(|e| MevError::Cli(CliError::Other(format!("http response body: {e}"))))?; | ||
|
|
||
| let bid: BuilderBidResponse = serde_json::from_slice(&body) | ||
| .map_err(|e| MevError::Cli(CliError::Other(format!("http response json: {e}"))))?; |
There was a problem hiding this comment.
Use existing CliError variants.
| if queued_tests.is_empty() { | ||
| return Err(CliError::Other("test case not supported".to_string())); | ||
| } |
There was a problem hiding this comment.
Use the _TestCaseNotSupported variant (remove the leading _)
crates/cli/src/commands/test/mev.rs
Outdated
| verdict: TestVerdict::Fail, | ||
| error: TestResultError::from_string("timeout/interrupted"), | ||
| ..TestResult::new("") | ||
| }); |
There was a problem hiding this comment.
Use the CliError::_TimeoutInterrupted variant (remove the leading _)
crates/cli/src/commands/test/mev.rs
Outdated
| }; | ||
|
|
||
| if resp.status().as_u16() > 399 { | ||
| return test_res.fail(CliError::Other(http_status_error(resp.status()))); | ||
| } |
There was a problem hiding this comment.
There is no support for credentials in v1.7.1.
Note that there are multiple instances of this pattern.
| timeout_token.cancel(); | ||
| }); | ||
|
|
||
| let start_time = Instant::now(); |
There was a problem hiding this comment.
Use tokio's time module to facilitate mocking time.
| let start_time = Instant::now(); | |
| let start_time = tokio::time::Instant::now(); |
Note that there are multiple instances of this pattern.
crates/cli/src/commands/test/mev.rs
Outdated
| ) -> Result<BeaconBlockMessage> { | ||
| let url = format!("{endpoint}/eth/v2/beacon/blocks/head"); | ||
| let (clean_url, creds) = parse_endpoint_credentials(&url)?; | ||
| let client = reqwest::Client::new(); | ||
|
|
||
| let resp = tokio::select! { | ||
| _ = token.cancelled() => return Err(CliError::Other("timeout/interrupted".to_string())), | ||
| r = apply_basic_auth(client.get(&clean_url), creds).send() => { | ||
| r.map_err(|e| CliError::Other(format!("http request do: {e}")))? | ||
| } | ||
| }; | ||
|
|
||
| let body = resp | ||
| .bytes() | ||
| .await | ||
| .map_err(|e| CliError::Other(format!("http response body: {e}")))?; | ||
|
|
||
| let block: BeaconBlock = serde_json::from_slice(&body) | ||
| .map_err(|e| CliError::Other(format!("http response json: {e}")))?; | ||
|
|
||
| Ok(block.data.message) | ||
| } |
There was a problem hiding this comment.
- No need to deal with credentials
- The token is actually not needed, the callers will interrupt this call.
- We already have
CliErrorvariants for these possible errors:
| ) -> Result<BeaconBlockMessage> { | |
| let url = format!("{endpoint}/eth/v2/beacon/blocks/head"); | |
| let (clean_url, creds) = parse_endpoint_credentials(&url)?; | |
| let client = reqwest::Client::new(); | |
| let resp = tokio::select! { | |
| _ = token.cancelled() => return Err(CliError::Other("timeout/interrupted".to_string())), | |
| r = apply_basic_auth(client.get(&clean_url), creds).send() => { | |
| r.map_err(|e| CliError::Other(format!("http request do: {e}")))? | |
| } | |
| }; | |
| let body = resp | |
| .bytes() | |
| .await | |
| .map_err(|e| CliError::Other(format!("http response body: {e}")))?; | |
| let block: BeaconBlock = serde_json::from_slice(&body) | |
| .map_err(|e| CliError::Other(format!("http response json: {e}")))?; | |
| Ok(block.data.message) | |
| } | |
| async fn latest_beacon_block(endpoint: &str) -> Result<BeaconBlockMessage> { | |
| let client = reqwest::Client::new(); | |
| let url = format!("{endpoint}/eth/v2/beacon/blocks/head"); | |
| let response = client.get(&url).send().await?; | |
| let body = response.bytes().await?; | |
| let block: BeaconBlock = serde_json::from_slice(&body)?; | |
| Ok(block.data.message) | |
| } |
| .map(|p| p.pubkey.clone()) | ||
| } | ||
|
|
||
| async fn get_block_header( |
There was a problem hiding this comment.
Similar comments to latest_beacon_block
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
There are multiple tests missing from testmev_internal_test.go. Let us known if you're going to include them in this PR or you'll add them in a follow up.
3 participants
Closes #235