refactor(core): replace ResourceBounds map with a fixed-field struct - #3819
Open
Ehsan-saradar wants to merge 3 commits into
Open
refactor(core): replace ResourceBounds map with a fixed-field struct#3819Ehsan-saradar wants to merge 3 commits into
Ehsan-saradar wants to merge 3 commits into
Conversation
Ehsan-saradar
force-pushed
the
refactor/3742-resource-bounds-struct
branch
from
July 16, 2026 07:00
3073074 to
ce1cc64
Compare
Swap map[Resource]ResourceBounds on the v3 transaction types for a ResourceBoundsMap struct with explicit L1Gas/L2Gas/L1DataGas fields, which is clearer and avoids a map allocation per transaction. To keep already-stored transactions readable without a DB migration, the type implements MarshalCBOR/UnmarshalCBOR that reproduce the exact legacy map[Resource]ResourceBounds wire format. l1_data_gas still only enters the transaction hash when present (non-nil MaxPricePerUnit), matching the old map-key check, so historical tx hashes are unchanged.
Replace *map[Resource]ResourceBounds on the feeder starknet.Transaction with a ResourceBoundsMap struct. JSON tags keep the uppercase L1_GAS/L2_GAS/L1_DATA_GAS feeder keys, and omitzero preserves the pre-0.13.4 shape where l1_data_gas is absent, so both add_transaction requests and decoded feeder responses round-trip unchanged. sn2core and the rpc feeder adapters consume the struct directly instead of iterating a map.
Ehsan-saradar
force-pushed
the
refactor/3742-resource-bounds-struct
branch
from
July 25, 2026 05:39
ce1cc64 to
16f2075
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors v3 transaction resource-bounds representation in both core and starknet from map[Resource]ResourceBounds to fixed-field structs (L1Gas / L2Gas / L1DataGas) to improve clarity and remove per-transaction map allocations, while preserving legacy encodings and hash behavior for backward compatibility.
Changes:
- Introduces
core.ResourceBoundsMapwith CBOR marshal/unmarshal that preserves the legacy on-diskmap[Resource]ResourceBoundsencoding, plus tests that pin byte-for-byte compatibility and hashing presence rules. - Introduces
starknet.ResourceBoundsMapfor feeder JSON with uppercase keys and omission behavior for absentl1_data_gas, plus JSON round-trip tests. - Updates adapters/RPC/VM/genesis/test utilities to use the new struct-based bounds representation end-to-end.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vm/transaction.go | Adapts core.ResourceBoundsMap into the VM payload map, conditionally including l1_data_gas. |
| starknet/transaction.go | Adds feeder-facing ResourceBoundsMap struct and updates Transaction.ResourceBounds type accordingly. |
| starknet/resource_bounds_test.go | Adds JSON round-trip tests to preserve feeder wire format, including pre-0.13.4 omission behavior. |
| rpc/v9/transaction.go | Migrates v9 RPC adaptation and feeder conversion paths to the new bounds struct types. |
| rpc/v9/transaction_test.go | Updates v9 transaction tests to construct core.ResourceBoundsMap literals. |
| rpc/v9/block_test.go | Updates v9 block tests to reference fixed fields instead of map indexing. |
| rpc/v8/transaction.go | Migrates v8 RPC adaptation and feeder conversion paths to the new bounds struct types. |
| rpc/v8/transaction_test.go | Updates v8 transaction tests to construct core.ResourceBoundsMap literals. |
| rpc/v8/subscriptions_test.go | Updates v8 subscription tests to use core.ResourceBoundsMap. |
| rpc/v8/block_test.go | Updates v8 block tests to reference fixed fields instead of map indexing. |
| rpc/v10/transaction_test.go | Updates v10 tests to construct core.ResourceBoundsMap literals. |
| rpc/v10/subscriptions_test.go | Updates v10 subscription tests to use core.ResourceBoundsMap. |
| rpc/v10/block_test.go | Updates v10 block tests to reference fixed fields instead of map indexing. |
| rpc/v10/adapt_transaction.go | Updates v10 core↔RPC↔feeder adaptation to use struct resource bounds. |
| genesis/genesis.go | Updates genesis transaction adaptation to produce core.ResourceBoundsMap. |
| core/transaction.go | Introduces core.ResourceBoundsMap, preserves CBOR wire format, and updates hashing to use fixed fields with presence semantics. |
| core/resource_bounds_test.go | Adds CBOR golden compatibility tests and hash presence-guard tests for l1_data_gas. |
| consensus/proposer/proposer_test.go | Updates proposer test transaction construction to use core.ResourceBoundsMap. |
| consensus/p2p/validator/transition_test.go | Updates validator transition tests to use core.ResourceBoundsMap. |
| clients/feeder/feeder_test.go | Updates feeder client tests to expect *starknet.ResourceBoundsMap instead of a map pointer. |
| adapters/testutils/transaction_test_utils.go | Updates test utilities to return core.ResourceBoundsMap instead of a map. |
| adapters/testutils/synctransaction_test_utils.go | Updates sync transaction builders to use zero-valued core.ResourceBoundsMap{} for pre-v3 transactions. |
| adapters/sn2core/sn2core.go | Updates feeder→core adaptation to build core.ResourceBoundsMap and preserve absence semantics. |
| adapters/sn2core/sn2core_test.go | Updates adapter tests to use core.ResourceBoundsMap. |
| adapters/p2p2core/transaction.go | Updates p2p→core transaction adaptation to populate core.ResourceBoundsMap. |
| adapters/core2p2p/transaction.go | Updates core→p2p transaction adaptation to read from core.ResourceBoundsMap fields. |
| .golangci.yaml | Excludes core.ResourceBounds / core.ResourceBoundsMap from exhaustruct checks due to meaningful zero values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rodrodros
reviewed
Jul 29, 2026
Comment on lines
+47
to
+53
| exhaustruct: | ||
| # These types have a meaningful zero value: a transaction with no resource | ||
| # bounds (v0-v2). Requiring every field to be spelled out at construction | ||
| # would only add noise, so exclude them from exhaustive-struct checking. | ||
| exclude: | ||
| - 'github.com/NethermindEth/juno/core\.ResourceBounds$' | ||
| - 'github.com/NethermindEth/juno/core\.ResourceBoundsMap$' |
Contributor
There was a problem hiding this comment.
Please remove the lint ignores
Contributor
Author
There was a problem hiding this comment.
Done — the exclude is gone and the repo is back to zero lint ignores.
Rather than spelling the zero fields out at all 14 call sites, I gave the zero values a name, following the existing core.EmptyStateDiff() helper: core.EmptyResourceBounds() and core.EmptyResourceBoundsMap(). Both list every field explicitly, so a resource added later still surfaces there, and exhaustruct keeps checking every real construction site.
No behaviour change — the returned values are identical to the literals they replace. gofmt, go build, golangci-lint run and the full test suite are green locally.
Mind approving the workflows when you get a chance? They are sitting on action_required.
…hem from lint
Turning the resource bounds map into a struct left `core.ResourceBoundsMap{}`
literals in the adapters, which exhaustruct flags because none of the fields
are spelled out. This was silenced with an `exhaustruct.exclude` entry, but
that switches the linter off for the type entirely: a resource added later
would no longer be reported at any construction site.
Drop the exclude and give the zero values a name instead, following the
existing `core.EmptyStateDiff` helper. `EmptyResourceBounds` and
`EmptyResourceBoundsMap` list every field explicitly, and the adapters call
them where a transaction carries no bounds at all (v0-v2) or a single
resource was absent from the payload (l1_data_gas before Starknet 0.13.4).
No behaviour change: the returned values are identical to the literals they
replace, and the repo keeps its zero lint ignores.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3742
Both
coreandstarknetresource bounds were modeled asmap[Resource]ResourceBounds. Since the keys are effectively fixed, this replaces them with a struct with explicitL1Gas/L2Gas/L1DataGasfields — clearer, and it drops a map allocation per transaction.Approach
core.ResourceBoundsMapstruct (L1Gas/L2Gas/L1DataGas) replacing themap[Resource]ResourceBoundsfield on the v3 transaction types, and the same for the feederstarknettype.MarshalCBOR/UnmarshalCBORthat keep the exact legacymap[Resource]ResourceBoundswire format. A golden test pins byte-for-byte equality across the three historical shapes — all three resources, the pre-0.13.4 two-resource form (nol1_data_gas), and the nil map on v0–v2 — so already-stored bytes still decode into the struct.l1_data_gasonly enters the tip/resources hash when it's actually set (non-nilMaxPricePerUnit), which reproduces the old map-key check, so pre-0.13.4 tx hashes stay identical.starknetstruct keeps the uppercaseL1_GAS/L2_GAS/L1_DATA_GASkeys and usesomitzero, so an absentl1_data_gasround-trips both ways.vmresource-bounds map as-is — it's the marshal-only Blockifier payload (with the shortL1_DATAkey), out of scope for this one.Testing
make lint— passcore,starknet,adapters/...,rpc/...,vm,genesis,consensus,clients/feeder) — pass, including the new backward-compat golden tests incore/resource_bounds_test.goandstarknet/resource_bounds_test.go.