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

Bump gas price at least by 1 gwei #13531

Closed
tomasklim opened this issue Jul 28, 2024 · 5 comments · Fixed by #15838
Closed

Bump gas price at least by 1 gwei #13531

tomasklim opened this issue Jul 28, 2024 · 5 comments · Fixed by #15838
Assignees
Labels
bug Something isn't working as expected good first issue Issue for newbie developers who want to participate

Comments

@tomasklim
Copy link
Member

tomasklim commented Jul 28, 2024

Before the implementation of #13328, the gas price was always increased by at least one gwei compared to the previous transaction. However, after the fix, the bump fee can propose an increase of just 1 wei. To maintain a more consistent fee bump logic, we should ensure that the new gas price is not only adjusted to the current network gas price but also is at least 1 gwei higher than the previous gas price.

Screenshot 2024-07-28 at 20 56 15

It should also fix this warning error (at least 2 gas price)

Screen.Recording.2024-07-28.at.20.50.06.mov
@tomasklim tomasklim added the bug Something isn't working as expected label Jul 28, 2024
@github-project-automation github-project-automation bot moved this to 🎯 To do in Suite Desktop Jul 28, 2024
@tomasklim tomasklim changed the title Add at least 1 gas price to bumped tx Bump gas price at least by 1 gwei Jul 29, 2024
@tomasklim tomasklim added the good first issue Issue for newbie developers who want to participate label Aug 11, 2024
@enjojoy enjojoy self-assigned this Sep 18, 2024
@enjojoy
Copy link
Contributor

enjojoy commented Dec 4, 2024

It already increases the gas by at least 1 gwei. I've also tested it with gas price higher than the proposed network gas and it's increased it by at least 1 gwei.

The current logic suggests that if the network gas is lower than the previous gas, the new gas will be increased by 1. Since it was already in gwei, it will be increased by 1 gwei.

const getEthereumFeeInfo = (info: FeeInfo, gasPrice: string) => {
    const current = new BigNumber(gasPrice);
    const minFeeFromNetwork = new BigNumber(fromWei(info.levels[0].feePerUnit, 'gwei'));

    const getFee = () => {
        if (minFeeFromNetwork.lte(current)) {
            return current.plus(1);
        }

        return minFeeFromNetwork;
    };

    const fee = getFee();

    // increase FeeLevel only if it's lower than predefined
    const levels = getFeeLevels('ethereum', info).map(l => ({
        ...l,
        feePerUnit: fee.toString(),
    }));

    return {
        ...info,
        levels,
        minFee: current.plus(1).toNumber(), // increase required minFee rate
    };
};

@tomasklim Please let me know if there is any other case in which the gas price can be increased by less than 1 gwei. Otherwise I would close the issue.

@tomasklim
Copy link
Member Author

Note: The minimum should be configurable because of new L2 chains #15478

@tomasklim
Copy link
Member Author

tomasklim commented Dec 7, 2024

@enjojoy Nobody touched the code from the time this issue was created.

Current develop (I see difference only 0.1)
Screenshot 2024-12-07 at 14 33 41

it is clear from the code you shared, that it can happen

    const getFee = () => {
        if (minFeeFromNetwork.lte(current)) {
            return current.plus(1);
        }

        return minFeeFromNetwork;
    };

Situation:
You send tx with gas price 10 current
Current network gas price is 10.5 minFeeFromNetwork

minFeeFromNetwork is gte than current

The gas price is set to 10.5, not to 11 (and as we update fees once per 10 block, it is better to have there a buffer)

It is incremented by 1 only in case the tx fee is higher than current gas price in the network (it can happen as we use legacy fees and also fees in suite can be old)

The inconsistency visible in the video is also still there.
Screenshot 2024-12-07 at 14 48 17
Screenshot 2024-12-07 at 14 48 12

@tomasklim tomasklim moved this from 🎯 To do to 🏃‍♀️ In progress in Suite Desktop Dec 9, 2024
@enjojoy
Copy link
Contributor

enjojoy commented Dec 9, 2024

@enjojoy Nobody touched the code from the time this issue was created.

Current develop (I see difference only 0.1) Screenshot 2024-12-07 at 14 33 41

it is clear from the code you shared, that it can happen

    const getFee = () => {
        if (minFeeFromNetwork.lte(current)) {
            return current.plus(1);
        }

        return minFeeFromNetwork;
    };

Situation: You send tx with gas price 10 current Current network gas price is 10.5 minFeeFromNetwork

minFeeFromNetwork is gte than current

The gas price is set to 10.5, not to 11 (and as we update fees once per 10 block, it is better to have there a buffer)

It is incremented by 1 only in case the tx fee is higher than current gas price in the network (it can happen as we use legacy fees and also fees in suite can be old)

The inconsistency visible in the video is also still there. Screenshot 2024-12-07 at 14 48 17 Screenshot 2024-12-07 at 14 48 12

Got it 🙏🏻 Thank you. Didn't realize that the difference with the network proposed fee can be less that 1 gwei and then it's problematic. Usually the fees are pretty high.

@github-project-automation github-project-automation bot moved this from 🏃‍♀️ In progress to 🤝 Needs QA in Suite Desktop Dec 11, 2024
@bosomt bosomt moved this from 🤝 Needs QA to ✅ Approved in Suite Desktop Dec 16, 2024
@bosomt
Copy link
Contributor

bosomt commented Dec 16, 2024

QA OK

Info:

  • Suite version: desktop 25.1.0 (0b11679)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/25.1.0 Chrome/128.0.6613.186 Electron/32.2.6 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T3T1 2.8.7 regular (revision e196413bb745443610211c325623ba7cee7309e9)
  • Transport: BridgeTransport 2.0.33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected good first issue Issue for newbie developers who want to participate
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants