dev: Better estimate fee test

[LucasLvy]

Instead of comparing hardcoded values, execute the transaction and compare the actual fees with the estimated fees

[kenkomu]

Hello, can I take this?

[tdelabro]

@kenkomu sure!!

Do you have a clear view on how to do that?

I think @LucasLvy was talking about tests in starknet-rpc-test/estimate_fee.rs but it would certainly be cool if you could also add such a test in the pallet tests.

[tdelabro]

@kenkomu you don't seem to be active. Comment again if you want to work on it again

[fishseabowl]

@tdelabro could I take it? Thanks

[kenkomu]

@kenkomu you don't seem to be active. Comment again if you want to work on it again

I want to work on it again.

[tdelabro]

@kenkomu What is your ETA?

[kenkomu]

What do you mean by ETA?
Can you guide me on how to tackle the task?

[tdelabro]

@kenkomu Estimated Time of Arrival: when do you think you will be finished? We gave you the issue two weeks ago and you haven't started it yet, so this time I would like you to commit to a schedule.

Instead of comparing hardcoded values, execute the transaction and compare the actual fees with the estimated fees

Lucas was referring tho this test:

starknet-rpc-test/estimate_fee.rs

#[rstest]
#[tokio::test]
async fn works_ok(madara: &ThreadSafeMadaraClient) -> Result<(), anyhow::Error> {
    let rpc = madara.get_starknet_client().await;

    let tx = BroadcastedInvokeTransaction {
        max_fee: FieldElement::ZERO,
        signature: vec![],
        nonce: FieldElement::ZERO,
        sender_address: FieldElement::from_hex_be(ACCOUNT_CONTRACT).unwrap(),
        calldata: vec![
            FieldElement::from_hex_be(TEST_CONTRACT_ADDRESS).unwrap(),
            get_selector_from_name("sqrt").unwrap(),
            FieldElement::from_hex_be("1").unwrap(),
            FieldElement::from(81u8),
        ],
        is_query: true,
    };

    let invoke_transaction = BroadcastedTransaction::Invoke(tx.clone());

    let invoke_transaction_2 =
        BroadcastedTransaction::Invoke(BroadcastedInvokeTransaction { nonce: FieldElement::ONE, ..tx });

    let estimates =
        rpc.estimate_fee(&vec![invoke_transaction, invoke_transaction_2], BlockId::Tag(BlockTag::Latest)).await?;

    // TODO: instead execute the tx and check that the actual fee are the same as the estimated ones
    assert_eq!(estimates.len(), 2);
    assert_eq!(estimates[0].overall_fee, 420);
    assert_eq!(estimates[1].overall_fee, 420);
    // https://starkscan.co/block/5
    assert_eq!(estimates[0].gas_consumed, 0);
    assert_eq!(estimates[1].gas_consumed, 0);

    Ok(())
}

As you can see at the end we do assert_eq against hardcoded values. A better approach, as said by lucas would be to execute the transaction and then compare the fee taken during the execution with those estimated by the call to estimate_fee and make sure they match.

Does it sound doable?

[kenkomu]

Yes, it sounds doable. Can you give me a week to complete it? I will be reaching out to you frequently.

[github-actions]

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

[tdelabro]

@kenkomu are you still working on this?

[kenkomu]

No, sorry for the late reply.

[tdelabro]

Don't worry no problem

[fishseabowl]

@tdelabro could I take it? Thanks

[tdelabro]

sure. Go ahead @fishseabowl

[github-actions]

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!