Update fee calculation logic to accept price #672

MitchTurner · 2024-02-08T01:26:11Z

Closes: #666
Closes: #667 (comment)

Partially completes: #664

fuel-tx/src/transaction/fee.rs

fuel-asm/src/args.rs

fuel-tx/src/transaction/fee.rs

This reverts commit e8ae067.

xgreenx · 2024-02-15T16:59:24Z

fuel-tx/src/tests/valid_cases/input.rs

@@ -495,7 +507,11 @@ fn message_message_coin() {

    let block_height = rng.gen();
    let err = tx
-        .check(block_height, &ConsensusParameters::standard())
+        .check(


Just a thought.

Hmm, it is the third time when we have updated a lot of places because of the modified number of arguments for the check function=D

Maybe it makes sense to accumulate all arguments inside of one type like CheckArgs.

Thinking about this more. There are no impls of check and only 4 impls of check_without_signature. I'm not too worried about it. It would be a lot more work rn to create a new type--all the places we call those functions. And those places would still need to be changed in the future.

This also goes back to the work I did a while ago breaking down the params so functions only use what they need. As soon as we start grouping things together again it gets harder to uphold the Interface Segregation Principle.

I'm pretty ambivalent about this one. I agree that it's a smell if we keep changing the interface, but I don't know if it's worth the work changing now.

xgreenx · 2024-02-15T17:01:57Z

fuel-tx/src/transaction/id.rs

@@ -277,7 +277,6 @@ mod tests {

    fn assert_id_common_attrs<Tx: Buildable>(tx: &Tx) {
        use core::ops::Deref;
-        assert_id_ne(tx, |t| t.set_gas_price(t.gas_price().not()));


Hmm, maybe we want to do the same for tips

Yeah. I was planning on doing a followup PR for tips.

I meant that you don't need to remove and add this part in the other PR. You simply can rename functions and you already are testing it. In this case, you will not lose the code that worked before.

It's okay to update the business logic in the next PR, but here we are testing serialization and deserialization that are the same.

xgreenx · 2024-02-15T17:06:58Z

fuel-tx/src/transaction/types/input/snapshot_tests.rs

@@ -19,7 +19,6 @@ fn tx_with_signed_coin_snapshot() {
            predicate: Empty::new(),
            predicate_data: Empty::new(),
        }))
-        .gas_price(1)


If you replace it with tip(1), you don't need to change the snapshots=)

The idea of this test to serialise all possible fields

Hmm. Looks like there still is a diff. I'm not sure what this assertion_line means.

fuel-tx/src/transaction/types/mint.rs

fuel-vm/src/checked_transaction.rs

xgreenx · 2024-02-15T21:32:49Z

fuel-vm/src/checked_transaction.rs

@@ -841,7 +863,6 @@ mod tests {
    // input
    #[quickcheck]
    fn min_fee_coin_input(


The purpose of this test to test different gas_price as well, could you return it back, please?=)

I removed it because it isn't used, since the gas_price isn't attached to the tx anymore.

But this test is for testing how fee calculation works in general, not how it works within the transaction.

xgreenx · 2024-02-15T21:33:01Z

fuel-vm/src/checked_transaction.rs

@@ -945,7 +964,6 @@ mod tests {
    // input
    #[quickcheck]
    fn min_fee_message_input(


The purpose of this test to test different gas_price as well, could you return it back, please?=)

xgreenx · 2024-02-15T21:34:41Z

fuel-vm/src/checked_transaction/builder.rs

@@ -20,27 +20,35 @@ where
    Tx: IntoChecked,
 {
    /// Finalize the builder into a [`Checked<Tx>`] of the correct type
-    fn finalize_checked(&mut self, height: BlockHeight) -> Checked<Tx>;
+    fn finalize_checked(&mut self, height: BlockHeight, gas_price: u64) -> Checked<Tx>;


It seems gas_price can be part of the builder as well along with block_height

gas_price isn't part of the transaction, though. It's part of the block.

It's true, but this type is used only in tests, and for simplicity it could be better=) But it is up to you since you need to update all palces in the fuel-core=)

I think I prefer it this way. I'll just have to deal with any costs on the fuel-core side :)

xgreenx · 2024-02-15T21:36:54Z

fuel-vm/src/interpreter/executors/main.rs

-    pub(crate) fn run(&mut self) -> Result<ProgramState, InterpreterError<S::DataError>> {
+    pub(crate) fn run(
+        &mut self,
+        gas_price: Word,


It seems can be part of the InterpreterParams along with another data=)

I guess this is a question I had about this api in general. Will each instance of the interpreter only use a single price? Do we never share an interpreter between blocks? That's why I went this route.

We always create a new instance of the Interpreter for each transaction. We don't have plans to share interpreter, since it is faster to allocate a new memory for Interpreter instead of clearing manually the old one, based on our benchmarks=)

Okay. I'm convinced that putting it on the Interpreter is a good idea, but I'm not going to put it on InterpreterParams because we have conversions from ConsensusParams that would need an additional parameter, if we just put it on Interpreter then we can keep those conversions.

I might rename InterpreterParams to InterpreterConsensusParams because that's a bit more accurate.

Hmm. I'm having a similar problem with Interpreter. We have some From conversions for other types that don't have gas_price so I'm going have to replace a bunch of from methods with new functions.

It might end up being a better interface long term to have it held on the Interpreter, but it's not obvious to me right now if that's the right design.

I don't hate having the extra param on the execution methods for now.

What about something like?

impl From<(ConsensusParameters, GasPrice)> for InterpreterParams {

Or maybe we just need to make a ::new constructor for InterpreterParams which accepts both ConsensusParams and GasPrice, instead of using a simple From conversion.

I thought about going with the tuple approach, but I don't feel like it is very idiomatic.

I'll give the constructor another shot.

xgreenx · 2024-02-15T21:37:15Z

fuel-vm/src/interpreter/executors/main.rs

@@ -692,6 +701,7 @@ where
    pub fn deploy(
        &mut self,
        tx: Checked<Create>,
+        gas_price: Word,


The same here, it can be part of the InterpreterParams

Voxelot · 2024-02-20T21:19:23Z

fuel-vm/src/interpreter.rs

-impl From<CheckPredicateParams> for InterpreterParams {
-    fn from(params: CheckPredicateParams) -> Self {
-        InterpreterParams {
+// impl From<ConsensusParameters> for InterpreterParams {


Should we remove this commented out code?

Gottit. Thanks.

Voxelot · 2024-02-21T00:20:25Z

fuel-vm/src/checked_transaction.rs

@@ -1606,8 +1703,14 @@ mod tests {
    where
        Tx: Chargeable + field::Inputs + field::Outputs,
    {
-        let available_balances =
-            balances::initial_free_balances(tx, gas_costs, fee_params, base_asset_id)?;
+        let arb_gas_price = 1;


Why did we stop using quickcheck to set the gas price?

Oh. I thought I fixed these spots. @xgreenx pointed it out but looks like I missed this one. Thanks.

fuel-vm/src/checked_transaction.rs

xgreenx · 2024-02-22T00:27:11Z

fuel-vm/src/tests/encoding.rs

@@ -217,7 +217,6 @@ fn transaction() {
            vec![0xfa],
            vec![0xfb, 0xfc],
            Policies::new()
-                .with_gas_price(Word::MAX >> 1)


Could you also use tip here instead of the removing it, please?=) It is related to the encoding, so it is okay to keep

Voxelot reviewed Feb 8, 2024

View reviewed changes

fuel-tx/src/transaction/fee.rs Outdated Show resolved Hide resolved

MitchTurner mentioned this pull request Feb 8, 2024

Rename the GasPrice Policy to Tips and implement it accordingly. #664

Closed

Voxelot reviewed Feb 8, 2024

View reviewed changes

fuel-asm/src/args.rs Show resolved Hide resolved

Voxelot reviewed Feb 8, 2024

View reviewed changes

fuel-tx/src/transaction/fee.rs Show resolved Hide resolved

xgreenx assigned MitchTurner Feb 12, 2024

MitchTurner mentioned this pull request Feb 12, 2024

Update specs fee calculation to include custom gas price FuelLabs/fuel-specs#544

Closed

MitchTurner added 13 commits February 12, 2024 16:22

WIP

8175147

Revert "WIP"

d1975eb

This reverts commit e8ae067.

Replace gas price policy with tip, add gas price to transact interface

bf31410

Update CHANGELOG

90f4729

fmt

f13e6bb

WIP update tests

ba0b1eb

WIP

9cbcadf

Fix run_script to have free gas

d1d11c8

Deal with test gas prices that must be zero

d431e61

Fix one test

ed7827b

Fix predicate test

c49d037

Fix output update test

2d307d2

Fix snapshot tests and some other gas price issues in tests

d9091fe

MitchTurner force-pushed the use-gas-price-in-fee-calc branch from 8c40fdc to d9091fe Compare February 13, 2024 00:23

MitchTurner and others added 7 commits February 13, 2024 10:51

Maybe fix test?

4ecb8fa

Fix create test

b80bd39

Removed todo

fe7bd46

Removed todo for tip GTF

3f5d950

Fix

b963c6b

Fix warnings

0f756c3

Make ci check changes

0c3c8ff

MitchTurner marked this pull request as ready for review February 14, 2024 18:13

MitchTurner requested a review from a team February 14, 2024 18:15

xgreenx reviewed Feb 15, 2024

View reviewed changes

Revert default impl changes

39a3036

MitchTurner requested a review from xgreenx February 17, 2024 19:03

MitchTurner added 6 commits February 19, 2024 09:55

Replace gas_price in prop tests

c548402

Replace some tip stuff for serialization and snapshots

32e187e

Merge branch 'master' into use-gas-price-in-fee-calc

f13085a

Fix function call

394d89a

Fix tests

935ba82

Add gas_price to InterpreterParams

cb6fb93

Voxelot reviewed Feb 20, 2024

View reviewed changes

Remove commented code

295152c

Voxelot reviewed Feb 21, 2024

View reviewed changes

MitchTurner and others added 3 commits February 20, 2024 17:16

Fix prop test to generate gas_price

e2a26e8

Merge branch 'master' into use-gas-price-in-fee-calc

9c1c3d2

Merge branch 'master' into use-gas-price-in-fee-calc

9dddcfe

xgreenx requested changes Feb 21, 2024

View reviewed changes

fuel-vm/src/checked_transaction.rs Outdated Show resolved Hide resolved

fuel-vm/src/checked_transaction.rs Outdated Show resolved Hide resolved

MitchTurner added 2 commits February 21, 2024 14:47

Remove gas_price from predicate execution functions

d7546e2

Fix tests

fd747ba

xgreenx reviewed Feb 22, 2024

View reviewed changes

MitchTurner added 2 commits February 21, 2024 16:30

Replace encoding stuff for tip

81e91b8

Replace the other encoding stuff

17cd96e

xgreenx approved these changes Feb 22, 2024

View reviewed changes

MitchTurner enabled auto-merge February 22, 2024 00:52

MitchTurner added this pull request to the merge queue Feb 22, 2024

Merged via the queue into master with commit 77f44f8 Feb 22, 2024
37 checks passed

MitchTurner deleted the use-gas-price-in-fee-calc branch February 22, 2024 01:02

xgreenx mentioned this pull request Feb 29, 2024

Release 0.47.0 #687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update fee calculation logic to accept price #672

Update fee calculation logic to accept price #672

MitchTurner commented Feb 8, 2024 •

edited

Loading

xgreenx Feb 15, 2024

MitchTurner Feb 16, 2024

MitchTurner Feb 19, 2024

xgreenx Feb 15, 2024

MitchTurner Feb 17, 2024 •

edited

Loading

xgreenx Feb 19, 2024

xgreenx Feb 15, 2024

xgreenx Feb 19, 2024

MitchTurner Feb 19, 2024

xgreenx Feb 15, 2024

MitchTurner Feb 15, 2024

xgreenx Feb 19, 2024

xgreenx Feb 15, 2024

xgreenx Feb 15, 2024

MitchTurner Feb 16, 2024

xgreenx Feb 19, 2024

MitchTurner Feb 19, 2024

xgreenx Feb 15, 2024

MitchTurner Feb 16, 2024

xgreenx Feb 19, 2024

MitchTurner Feb 19, 2024

MitchTurner Feb 19, 2024

Voxelot Feb 20, 2024 •

edited

Loading

MitchTurner Feb 20, 2024

xgreenx Feb 15, 2024

Voxelot Feb 20, 2024

MitchTurner Feb 20, 2024

Voxelot Feb 21, 2024

MitchTurner Feb 21, 2024

xgreenx Feb 22, 2024

Update fee calculation logic to accept price #672

Update fee calculation logic to accept price #672

Conversation

MitchTurner commented Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner commented Feb 8, 2024 •

edited

Loading

MitchTurner Feb 17, 2024 •

edited

Loading

Voxelot Feb 20, 2024 •

edited

Loading