Skip to content

fix(precompile-storage): replace unimplemented! panics with structured errors in SetHandler transient methods#3281

Open
eric-ships wants to merge 1 commit into
mainfrom
ericliu/bop-286-12-transient-sett-operations-panic-instead-of-returning
Open

fix(precompile-storage): replace unimplemented! panics with structured errors in SetHandler transient methods#3281
eric-ships wants to merge 1 commit into
mainfrom
ericliu/bop-286-12-transient-sett-operations-panic-instead-of-returning

Conversation

@eric-ships
Copy link
Copy Markdown
Contributor

BOP-286

Motivation

SetHandler<T> implemented the Handler<T> transient methods (t_read, t_write, t_delete) with unimplemented!(), causing a process panic if any generic caller invoked them. The Handler<T> trait returns Result precisely so callers can handle unsupported operations without crashing.

The practical risk is a node crash rather than a clean revert: a future developer adding a Set field to a transient storage struct, or a generic caller iterating over handler methods, would cause an unrecoverable panic at runtime instead of receiving a structured error. Even though transient Set storage is unlikely by design, the fix is a 3-line change that permanently closes this crash vector. The panic vs. Err(Fatal) distinction matters for operational stability: Fatal surfaces to revm's frame handler, while a panic kills the process.

What changed

Replaced the three unimplemented!() stubs in SetHandler<T>'s Handler impl with Err(BasePrecompileError::Fatal("Set does not support transient storage".into())). Added a test asserting all three methods return Err.

…s in SetHandler transient methods

SetHandler<T> implemented t_read, t_write, and t_delete with unimplemented!(),
causing a process panic instead of returning Err. Replace each stub with
Err(BasePrecompileError::Fatal(...)) and add a test asserting all three
transient methods return Err.
@cb-heimdall
Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@linear
Copy link
Copy Markdown

linear Bot commented Jun 6, 2026

BOP-286

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 6, 2026

Review Summary

No issues found. This is a clean, well-motivated change.

The replacement of unimplemented!() with Err(BasePrecompileError::Fatal(...)) is the correct fix:

  • Fatal is the appropriate error variant — it's documented as "Unrecoverable internal error" and is consistent with how Set::store() already handles its own unsupported operation (line 129).
  • All other Handler implementations (Slot, VecHandler, ArrayHandler, BytesLikeHandler) provide real transient storage support by delegating to Slot's TransientOps. SetHandler is the only one that cannot support transient storage due to its dual-structure invariants (values Vec + positions Mapping), so explicitly returning an error is the right approach.
  • The test covers all three methods and uses is_err() which is sufficient for this case.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 6, 2026

✅ base-std fork tests: all 602 passed

base/base is fully in sync with the base-std spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants