Update rain.error version #419
Conversation
WalkthroughUpdated workspace dependency Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-06-17T10:56:40.904ZApplied to files:
📚 Learning: 2025-06-17T10:56:40.904ZApplied to files:
📚 Learning: 2025-06-17T10:56:40.904ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)crates/eval/src/fork.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: git-clean
🔇 Additional comments (2)
crates/eval/src/fork.rs (1)
245-250: Clarify the purpose of the new parameter and verifyNoneis appropriate.The second parameter has been added to
selector_registry_abi_decode, but its purpose is unclear from the PR description (which is empty). Please clarify:
- What does this new parameter control?
- Is
Nonethe correct value for this use case, or should it be configured based on context?- Are there scenarios where a non-
Nonevalue would be more appropriate?Consider adding a comment explaining when this parameter should be non-
None:if decode_error && raw.exit_reason == InstructionResult::Revert { // decode result bytes to error selectors if it was a revert + // Second parameter is the custom registry; None uses the default registry return Err(ForkCallError::AbiDecodedError( AbiDecodedErrorType::selector_registry_abi_decode(&raw.result, None).await?, )); }Cargo.toml (1)
37-37: Verify git revision accessibility and build compatibility.All call sites to
selector_registry_abi_decodein the codebase have been identified and are consistent:
crates/eval/src/fork.rslines 248 and 293: Both useAbiDecodedErrorType::selector_registry_abi_decode(&raw.result, None).await?Both calls use the same 2-parameter signature consistently. However, the following require manual verification:
- Confirm the git revision
3d2ed70fb2f7c6156706846e10f163d1e493a8d3exists in the rainlanguage/rain.error repository- Run
cargo checkorcargo buildto verify the dependency resolves and the API signature is compatible with the updated crate
| if decode_error && raw.exit_reason == InstructionResult::Revert { | ||
| // decode result bytes to error selectors if it was a revert | ||
| return Err(ForkCallError::AbiDecodedError( | ||
| AbiDecodedErrorType::selector_registry_abi_decode(&raw.result).await?, | ||
| AbiDecodedErrorType::selector_registry_abi_decode(&raw.result, None).await?, | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM - Consistent with the change in alloy_call.
This change mirrors the update at line 248 and maintains consistency across both error-decoding paths. The same questions about the parameter's purpose apply here.
Consider extracting the error decoding logic to reduce duplication between alloy_call and alloy_call_committing:
async fn decode_abi_error(raw: &RawCallResult) -> Result<AbiDecodedErrorType, anyhow::Error> {
AbiDecodedErrorType::selector_registry_abi_decode(&raw.result, None).await
}Then use it in both functions:
if decode_error && raw.exit_reason == InstructionResult::Revert {
return Err(ForkCallError::AbiDecodedError(decode_abi_error(&raw).await?));
}There was a problem hiding this comment.
@findolor Understood, I'll skip this refactoring. Thanks for the feedback!
🧠 Learnings used
Learnt from: 0xgleb
PR: rainlanguage/rain.interpreter#334
File: crates/eval/src/fork.rs:489-494
Timestamp: 2025-06-17T10:56:40.904Z
Learning: In the foundry-evm crate, the `replay_until` method on the backend returns an error type that is convertible to `ForkCallError`, so using the `?` operator works without requiring explicit error mapping.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Chores
Bug Fixes