Skip to content
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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

RPC update - add l2 gas #2335

wants to merge 20 commits into from

Conversation

AnkushinDaniil
Copy link
Contributor

@AnkushinDaniil AnkushinDaniil commented Dec 18, 2024

Fix #2323
Fix #2324
Fix #2334

Implement RPC changes regarding l2_gas

Core Data Structures and Adaptation Functions:

  • core/block.go: Updated the Header struct to include L1GasPriceETH, L2GasPriceETH, L1GasPriceSTRK, and L2GasPriceSTRK fields, replacing the previous GasPrice and GasPriceSTRK fields. [1] [2]
  • adapters/p2p2core/block.go: Updated the AdaptBlockHeader function to include the new L1 and L2 gas price fields.
  • adapters/sn2core/sn2core.go: Adjusted the AdaptBlock function to handle the new gas price fields.

RPC:

  • rpc/block.go: Updated the BlockHeader struct and adaptBlockHeader function to include L2 gas prices. [1] [2]
  • rpc/estimate_fee.go: Introduced a new FeeEstimate struct to handle L1 and L2 gas prices and added a versioned EstimateFeeV0_7 function for backward compatibility. [1] [2] [3]

Test Cases:

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 79.54545% with 18 lines in your changes missing coverage. Please review.

Project coverage is 74.75%. Comparing base (4ba933f) to head (dfa0789).

Files with missing lines Patch % Lines
rpc/estimate_fee.go 51.85% 12 Missing and 1 partial ⚠️
rpc/simulation.go 82.14% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2335      +/-   ##
==========================================
+ Coverage   74.42%   74.75%   +0.33%     
==========================================
  Files         110      110              
  Lines       11771    11818      +47     
==========================================
+ Hits         8760     8834      +74     
+ Misses       2329     2306      -23     
+ Partials      682      678       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

if b.L1GasPrice != nil {
return b.L1GasPrice.PriceInFri
}
return b.GasPriceFRI
}

// TODO: Fix when we have l2 gas price
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what did you mean by that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the feeder gateway doesn't send this field. It can be removed once we have it from the FGW and have field validation in the proto fields.

@AnkushinDaniil AnkushinDaniil added the RPC JSON RPC API label Dec 20, 2024
@AnkushinDaniil AnkushinDaniil changed the title Daniil/update rpc RPC update - add l2 gas Dec 20, 2024
@AnkushinDaniil AnkushinDaniil added the go Pull requests that update Go code label Dec 20, 2024
rpc/estimate_fee.go Outdated Show resolved Hide resolved
rpc/block.go Outdated Show resolved Hide resolved
Copy link
Contributor

@weiihann weiihann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// Amount of WEI charged per Gas spent on L1
L1GasPriceETH *felt.Felt
// Amount of STRK charged per Gas spent on L2
L2GasPriceETH *felt.Felt
Copy link
Contributor

@kirugan kirugan Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it's not as simple as that. When we encode structures into bytes encoder it stores field names alongside their values so if we rename it here we receive nil in these fields even if our db is up to date.

@IronGauntlets please confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be fixed with tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked on block №800:

        "l1_gas_price": {
            "price_in_fri": "0x0",
-           "price_in_wei": "0x476b2446"
+           "price_in_wei": null
        },

@kirugan you are right

return nil, httpHeader, err
}

return utils.Map(result, func(tx SimulatedTransaction) FeeEstimateV0_7 {
Copy link
Contributor

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)

Copy link
Contributor Author

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.

rpc/estimate_fee.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code RPC JSON RPC API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC08 - Execution_resources changes RPC08 - Fee_estimate changes RPC08 - Block header now contains l2_gas
3 participants