-
Notifications
You must be signed in to change notification settings - Fork 11
Update starknet header to include l2 gas price #197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds an optional L2 gas price field to Starknet block headers: a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/starknet/src/block.ts (1)
173-199: Codec wiring is correct; consider clarifying thel2GasPriceJSDoc
l2GasPrice: OptionalCodec(ResourcePrice)is consistent with how L1 gas prices are modeled and correctly keeps the field optional. The JSDoc line@prop l2GasPrice Calldata gas price.looks copy‑pasted froml1GasPrice; consider rephrasing it (e.g., “L2 gas price”) so it’s clear this is for L2, not L1 calldata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
change/@apibara-starknet-79e122c4-6fa1-458b-bef9-d5a2038f8670.json(1 hunks)packages/starknet/proto/data.proto(2 hunks)packages/starknet/src/block.ts(2 hunks)packages/starknet/src/proto/data.ts(7 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). (1)
- GitHub Check: test
🔇 Additional comments (3)
packages/starknet/proto/data.proto (1)
31-55: L2 gas price field is correctly added to BlockHeader
l2_gas_priceuses an unused field number (11), matches the existingResourcePricetype used for L1 gas fields, and keeps the message backward compatible. No issues here.change/@apibara-starknet-79e122c4-6fa1-458b-bef9-d5a2038f8670.json (1)
1-7: Prerelease metadata matches the changeChange file correctly describes a patch-level update for
@apibara/starknetwith an accurate comment. Looks good.packages/starknet/src/proto/data.ts (1)
310-355:BlockHeader.l2GasPriceis consistently wired through interface, defaults, encode/decode, and JSON helpersThe new
l2GasPricefield is:
- Optional at the type level (
ResourcePrice | undefined),- Initialized to
undefinedincreateBaseBlockHeader,- Encoded/decoded with field number 11 (tag 90) using
ResourcePrice,- Properly handled in
fromJSON,toJSON, andfromPartial.This mirrors the existing L1 gas price fields and preserves binary and JSON compatibility. No issues found.
Also applies to: 1083-1320
dcefb7d to
c555fbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/starknet/src/block.ts (1)
173-186: Documentingl2GasPricelooks consistent; consider clarifying unitsThe new
@prop l2GasPrice L2 gas price.entry is consistent with the field name and placement. If you want to be extra explicit, you could mirrorResourcePrice’s doc and mention that the value is expressed infri/wei, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
change/@apibara-starknet-79e122c4-6fa1-458b-bef9-d5a2038f8670.json(1 hunks)packages/starknet/proto/data.proto(2 hunks)packages/starknet/src/block.ts(2 hunks)packages/starknet/src/proto/data.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/starknet/src/proto/data.ts
- packages/starknet/proto/data.proto
- change/@apibara-starknet-79e122c4-6fa1-458b-bef9-d5a2038f8670.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/starknet/src/block.ts (1)
packages/starknet/src/proto/data.ts (2)
ResourcePrice(618-625)ResourcePrice(4730-4802)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (1)
packages/starknet/src/block.ts (1)
187-199:l2GasPricecodec wiring is correct and backward‑compatibleAdding
l2GasPrice: OptionalCodec(ResourcePrice)toBlockHeadermatches the new proto field and keeps older blocks decodable (field simply undefined when absent). This looks correct and in line with how other optional header fields are modeled.
This PR adds the field. It's marked optional so that it works on older streams.