Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

feat: Return fee estimate that make sense #1573

Closed
tdelabro opened this issue Apr 26, 2024 · 11 comments
Closed

feat: Return fee estimate that make sense #1573

tdelabro opened this issue Apr 26, 2024 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@tdelabro
Copy link
Collaborator

Feature Request

Describe the Feature Request

Right now all our fee estimation methods return mocked/false values. We should fix that.

Describe Preferred Solution

simulate_transaction

#329 is still not fixed, but it can be now
replace

            .map(|x| FeeEstimate {
               >>>> gas_price: FieldElement::from(10u128),
               <<<< gas_price: fee_estimate.0.try_into().map_err(|_| StarknetRpcApiError::InternalServerError)?,
                gas_consumed: FieldElement::from(x.1),
                overall_fee: FieldElement::from(x.0),
                unit: PriceUnit::Wei,
            })

get_block-like RPCs

Right now the code contains in multiple places:

            // TODO: fill real prices
            l1_gas_price: ResourcePrice { price_in_fri: Default::default(), price_in_wei: Default::default() },

Instead, get the price from the block header gas_prices field, it contains eth_l1_gas_price and strk_l1_gas_price, which are the values we are looking for.

Estimate fee

Right now we are totally wrong on our fee estimation. We return (exec_info.actual_fee.0, *l1_gas_usage) in crates/pallets/starknet/src/simulations.rs as values for overal_fee and gas_consumed. That is not how the protocol is supposed to work: https://docs.starknet.io/documentation/architecture_and_concepts/Network_Architecture/fee-mechanism/
We should follow this specifications, for this we can take inspiration of the pathfinder implementation here: https://github.com/eqlabs/pathfinder/blob/main/crates/executor/src/estimate.rs#L12

@tdelabro tdelabro added the enhancement New feature or request label Apr 26, 2024
@apoorvsadana
Copy link
Collaborator

Hey, I can pick this up

@apoorvsadana apoorvsadana self-assigned this Apr 29, 2024
@apoorvsadana
Copy link
Collaborator

apoorvsadana commented Apr 29, 2024

I am thinking of this logic to fetch the L1 gas price per block

  1. Create a new inherent to set_l1_gas_price similar to sequencer address
  2. Use the eth_config being created in service.rs here - https://github.com/apoorvsadana/madara/blob/main/crates/node/src/service.rs#L467-L467 outside
  3. In create_inherent_data_providers - https://github.com/apoorvsadana/madara/blob/main/crates/node/src/service.rs#L521-L521 fetch the gas price and push it into the inherent

wdyt?

@tdelabro
Copy link
Collaborator Author

In create_inherent_data_providers - https://github.com/apoorvsadana/madara/blob/main/crates/node/src/service.rs#L521-L521 fetch the gas price and push it into the inherent

Where will you be fetching the gas price from? Will you be doing some http request?
Wouldn't it be better to maintain an in-memory value last_observed_gas_price and use it when needed?

@apoorvsadana
Copy link
Collaborator

Yes over an http request.
Where will the variable last_observed_gas_price be exactly?

@tdelabro
Copy link
Collaborator Author

Won't it delay the block production by the time of the http request?
It could be a some sort of global const in the crate? With a worker updating it

@apoorvsadana
Copy link
Collaborator

Won't it delay the block production by the time of the http request? It could be a some sort of global const in the crate? With a worker updating it

Hmm, fair point. So a worker updates some const every X seconds and the start of a new block, an inherent fetches the latest storage value and inserts it into the runtime?

@tdelabro
Copy link
Collaborator Author

Hmm, fair point. So a worker updates some const every X seconds and the start of a new block, an inherent fetches the latest storage value and inserts it into the runtime?

That is what I would say. The gas price is updated every eth block, no? We are listening to them anyway for messages, no?
Or do we need to call some offchain oracle to get the price?

@apoorvsadana
Copy link
Collaborator

On eth, the gas price isn't a part of the block header afaik. It's just a metric based on the supply and demand.

@tdelabro
Copy link
Collaborator Author

Decentralizing this will be a terrible experience...

@apoorvsadana
Copy link
Collaborator

Why? The author node can set the l1_gas_price based however it likes, other nodes can reject it if they feel its incorrect (similar to the timestamp inherent)

@apoorvsadana
Copy link
Collaborator

Resolved with #1601

@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants