feat(fbal): FBAL Builder database wrapper #322
Conversation
✅ Heimdall Review Status
|
| sol!( | ||
| #[sol(rpc)] | ||
| AccessListContract, | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/../test-utils/contracts/out/AccessList.sol/AccessList.json" | ||
| ) | ||
| ); |
There was a problem hiding this comment.
I'm not sure of the best way to organize this - we should brainstorm a bit - but I don't think it generally makes sense to have individual crates depend on test-utils in any way. If we need to use the accesslist.json here, we should probably localize it and then re-export the type under the test-utils feature flag so the test-utils crate can re-use this type.
Not a blocker for this PR, but worth investigation and coming to a consensus
There was a problem hiding this comment.
😅 hahah. yeah this repo used to have localized testing setups not too long ago, the test-utils consolidation is fairly recent.
note however the JSON is a build artifact of contracts that live within test-utils. I originally did have contracts at the repo root which tbh i preferred, but those got shifted to the test-utils crate so now we have this situation. I don't think we need to re-export the type in hopes test-utils will ever use it - the dependency relationship is almost always X (depends on) test-utils (for harnesses, fixtures, etc) and very rarely the other way around
meyer9
left a comment
meyer9
left a comment
There was a problem hiding this comment.
LGTM! Left a few non-blocking comments.
| let account_changes = self.access_list.changes.entry(*address).or_default(); | ||
|
|
||
| // Update balance, nonce, and code | ||
| match self.db.basic(*address)? { |
There was a problem hiding this comment.
Could also check account.is_touched() before running this DB lookup (very small perf optimization)
There was a problem hiding this comment.
I'll try it out.
Per the spec we need to have an entry in AccountChanges even for addresses that were simply loaded/read without having any modifications made (just w/ empty changes though). I'm wondering if there's any case where is_touched is false but we also haven't previously filed an entry for that account via our wrapper around db.basic(...)?
| /// Maximum txn index from the full block that's included in this access list | ||
| pub max_tx_index: u64, | ||
| /// keccak256 hash of the RLP-encoded account changes list | ||
| pub fal_hash: B256, |
There was a problem hiding this comment.
I know the original EIP had access list hash stored in the header to ensure the access list was determined by the time the block was built, but I don't necessarily see the point of including the hash here - do you have a specific use case in mind? Can also address in a future PR!
There was a problem hiding this comment.
not as of today, think it's an artifact of having modified the BAL spec. if necessary we can remove this later, but will think about it a bit more
|
|
||
| #[test] | ||
| /// Tests that a single ETH transfer is included in the access list | ||
| fn test_single_transfer() { |
There was a problem hiding this comment.
can you add a test for sending 0 balance? In this case, the receiving account shouldn't be included in the access list since it hasn't changed, but the account will be considered touched.
There was a problem hiding this comment.
feel free to take or leave since we're not even using account.is_touched right now.
There was a problem hiding this comment.
that's interesting, i'll add it in a followup alongside some other fixes that need to be made
|
Review Error for meyer9 @ 2026-01-09 15:38:44 UTC |
* Fix builder tx cumulative da * fix builder gas and da calculation * add test
Alternative implementation for a FBAL Builder instead of #273. Contains all the same test cases (+ improved with having stronger assertions). Retains all the same functionality so far.
Makes it easier to build custom Payload Builders and Block Executors with this pattern rather than the
revminspector approach used previously.Per the spec, there are some minor bugs that remain to be fixed that were also bugs in the older PR #273. For now, this PR is 1:1 compatible with the older PR. Additional identified bugs will be resolved in a followup.