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

The help comments for --gas-limit and --block-gas-limit are the same. #4693

Closed
1 of 2 tasks
minaminao opened this issue Apr 4, 2023 · 2 comments · Fixed by #9406
Closed
1 of 2 tasks

The help comments for --gas-limit and --block-gas-limit are the same. #4693

minaminao opened this issue Apr 4, 2023 · 2 comments · Fixed by #9406
Labels
A-docs Area: docs C-forge Command: forge Cmd-forge-script Command: forge script Cmd-forge-test Command: forge test P-low Priority: low T-bug Type: bug
Milestone

Comments

@minaminao
Copy link
Contributor

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (0e7ff88 2023-04-04T00:11:28.439758000Z)

What command(s) is the bug in?

forge test --help, forge script --help

Operating System

macOS (Intel)

Describe the bug

The help messages for --gas-limit and --block-gas-limit are confusing because they are the same.

$ forge test --help | grep gas-limit -A 1
      --gas-limit <GAS_LIMIT>
          The block gas limit
--
      --block-gas-limit <GAS_LIMIT>
          The block gas limit

The following code shows that the --block-gas-limit option is prioritized.

    pub fn gas_limit(&self) -> U256 {
        self.env.block_gas_limit.unwrap_or(self.env.gas_limit).into()
    }

When I actually specified both and got the value of --block-gas-limit for block.gaslimit. If only one was specified, that value was given.

If they are equivalent, it would be easier to understand if one or the other is deleted. If not, it would be better to change the help message.

@minaminao minaminao added the T-bug Type: bug label Apr 4, 2023
@gakonst gakonst added this to Foundry Apr 4, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Apr 4, 2023
@mattsse
Copy link
Member

mattsse commented Apr 4, 2023

I'm not sure the gas-limit isn't actually intended for the transaction.

but this is redundant atm, but not sure what the ideal solution here is.

what do you think?

@minaminao
Copy link
Contributor Author

@mattsse Thanks for your comment! Do you mean that --gas-limit may have been created as the gas limit for a transaction, but the option is redundant now because only one transaction is executed per block in the current test and script?

Cutting it short, I think it would be better to keep gas_limit and remove the --gas-limit option.

It's difficult for users to guess from the current help message if the --gas-limit was designed as a future role for the transaction gas limit, but now behaves as a block gas limit. In cast, --gas-limit is the transaction gas limit, and it's not trivial that specifying --gas-limit changes the block.gaslimit. I think the behavior of the --gas-limit option in forge and cast should be consistent.

Also, I don't currently need the transaction gas limit option in my tests and scripts. I think the block gas limit option is sufficient. I can think of cases where it's necessary to specify a block gas limit, but I can't think of any cases where it's necessary to specify a transaction gas limit using the option. This is because the function call option {gas: <foo>} can be used to specify the transaction gas limit. (related: #2627)

However, I think the implementation with gas_limit and block_gas_limit should be left as is if there is even a slight possibility that the the roles of two vars will have differences in the future. The performance will not change if gas_limit and block_gas_limit are present. I think it would be a waste to reimplement them in the future.

@zerosnacks zerosnacks added A-docs Area: docs Cmd-forge-test Command: forge test C-forge Command: forge Cmd-forge-script Command: forge script labels Jul 1, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@grandizzy grandizzy added the P-low Priority: low label Oct 29, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: docs C-forge Command: forge Cmd-forge-script Command: forge script Cmd-forge-test Command: forge test P-low Priority: low T-bug Type: bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants