-
Notifications
You must be signed in to change notification settings - Fork 185
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
RPC update - add l2 gas #2335
base: main
Are you sure you want to change the base?
RPC update - add l2 gas #2335
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2335 +/- ##
==========================================
+ Coverage 74.62% 74.93% +0.30%
==========================================
Files 110 110
Lines 12024 12072 +48
==========================================
+ Hits 8973 9046 +73
+ Misses 2354 2332 -22
+ Partials 697 694 -3 ☔ View full report in Codecov by Sentry. |
0b11ce6
to
49278bf
Compare
bfeb90f
to
5bf7f2b
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.
LGTM
return nil, httpHeader, err | ||
} | ||
|
||
return utils.Map(result, func(tx SimulatedTransaction) FeeEstimateV0_7 { |
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.
you can pass function name (=pointer) like:
return utils.Map(result, FeeEstimateToV0_7)
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.
I don't think so, feeEstimateToV0_7
adapts FeeEstimate
, not SimulatedTransaction
.
return utils.Map(result, feeEstimateToV0_7)
requires changes in feeEstimateToV0_7
.
dfa0789
to
eed5b3e
Compare
a476172
to
fccabd3
Compare
968275a
to
c1abd7a
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.
Half review. Will continue tomorrow
adapters/core2p2p/block.go
Outdated
L2GasPriceFri: AdaptUint128(header.L2GasPriceSTRK.NilToZero()), | ||
L2GasPriceWei: AdaptUint128(header.L2GasPriceETH.NilToZero()), |
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.
If this values cannot be nil they should be handled explicitly up top.
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.
c1abd7a
to
1326b54
Compare
1326b54
to
6dcb2f5
Compare
Fix #2323
Fix #2324
Fix #2334
Implement RPC changes regarding
l2_gas
Core Data Structures and Adaptation Functions:
core/block.go
: Updated theHeader
struct to includeL1GasPriceETH
,L2GasPriceETH
,L1GasPriceSTRK
, andL2GasPriceSTRK
fields, replacing the previousGasPrice
andGasPriceSTRK
fields. [1] [2]adapters/p2p2core/block.go
: Updated theAdaptBlockHeader
function to include the new L1 and L2 gas price fields.adapters/sn2core/sn2core.go
: Adjusted theAdaptBlock
function to handle the new gas price fields.RPC:
rpc/block.go
: Updated theBlockHeader
struct andadaptBlockHeader
function to include L2 gas prices. [1] [2]rpc/estimate_fee.go
: Introduced a newFeeEstimate
struct to handle L1 and L2 gas prices and added a versionedEstimateFeeV0_7
function for backward compatibility. [1] [2] [3]Test Cases:
adapters/sn2core/sn2core_test.go
: Updated test cases to assert the new gas price fields.rpc/block_test.go
: Added assertions for L2 gas prices in the relevant test cases. [1] [2] [3]rpc/estimate_fee_test.go
: Included tests for the newEstimateFeeV0_7
function.rpc/events_test.go
: Updated expected JSON responses to include L2 gas prices. [1] [2]