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

AUTO-8804: create chain specific modules for l1 gas calculations #11896

Merged
merged 23 commits into from
Feb 14, 2024

Conversation

FelixFan1992
Copy link
Contributor

No description provided.

Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

Comment on lines 578 to 582
if (isExecution) {
l1CostWei = hotVars.chainSpecificModule.getL1Fee(msg.data);
} else {
// if it's not performing upkeeps, use gas ceiling multiplier to estimate the upper bound
l1CostWei = hotVars.gasCeilingMultiplier * hotVars.chainSpecificModule.getMaxL1Fee(s_storage.maxPerformDataSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

contracts/src/v0.8/automation/dev/chains/ScrollModule.sol Outdated Show resolved Hide resolved
IScrollL1GasPriceOracle internal constant SCROLL_ORACLE = IScrollL1GasPriceOracle(SCROLL_ORACLE_ADDR);

function blockHash(uint256 blocknumber) external view returns (bytes32) {
return blockhash(blocknumber);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if this returns an empty bytes32 or throws an error if the block number is older than 256? If it's the latter, we might need to do the same thing we do for arbitrum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's be defensive here and add this check.
will check later.

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

1 similar comment
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@@ -7,12 +7,17 @@ import { AutomationRegistryLogicB2_2__factory as AutomationRegistryLogicBFactory
import { IAutomationRegistryMaster as IAutomationRegistry } from '../../../typechain/IAutomationRegistryMaster'
import { IAutomationRegistryMaster__factory as IAutomationRegistryMasterFactory } from '../../../typechain/factories/IAutomationRegistryMaster__factory'

const ZERO = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had to add these bc SonarQube thinks using magic numbers 0, 1, 2 directly causes brain overload

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lololol you've got to be kidding me!? can we make fix sonar cube rather than making ZERO constants!? that's cray.

Comment on lines 31 to 33
function getL1Fee(bytes calldata) external view returns (uint256) {
return ARB_GAS.getCurrentTxL1GasFees();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function ignores the calldata and just uses the msg.data, right? This feels like a confusing API imho. Can we change this function to actually estimate the gas for the provided data?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should change the interface to instead be getCurrentL1Fee() or similar and avoid passing data all together? Each module can read msg.data directly.

contracts/src/v0.8/automation/dev/chains/ScrollModule.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/automation/dev/chains/ScrollModule.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@infiloop2 infiloop2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Some minor improvements on chain modules which we can do in followup PRs

@infiloop2 infiloop2 dismissed RyanRHall’s stale review February 14, 2024 11:46

Synced offline, got approval

@cl-sonarqube-production
Copy link

@infiloop2 infiloop2 added this pull request to the merge queue Feb 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2024
@infiloop2 infiloop2 added this pull request to the merge queue Feb 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2024
@infiloop2 infiloop2 added this pull request to the merge queue Feb 14, 2024
Merged via the queue into develop with commit 9a687b0 Feb 14, 2024
105 checks passed
@infiloop2 infiloop2 deleted the AUTO-8804 branch February 14, 2024 14:39
momentmaker added a commit that referenced this pull request Feb 15, 2024
* develop: (74 commits)
  VRF zero confirmation delay  (#11947)
  add toml configs to paths that can cause e2e tests to run in ci (#12001)
  bump golang.org/x/... (#12042)
  [chore] Replace clock with specialized lib (#12031)
  Update style guide (#12041)
  plugins/cmd/chainlink-mercury: (re)move to chainlink-data-streams repo (#11994)
  bump go-plugin (#12033)
  Adds timeout on fuzz script execution (#12024)
  Add bytes type to abi_type (#12029)
  AUTO-8804: create chain specific modules for l1 gas calculations (#11896)
  VRF-878 Gas Optimization V2 Plus (#11982)
  Improving deletes performance by limiting number of records to scan (#12007)
  core/web: improve health CLI readabilty (#12021)
  Handle a 0 exit code from the remote runner instead of always failing (#12015)
  Add a simple Codec test (#12006)
  Allow for custom config poller onchain codec (LLO support) (#11957)
  Update Sonar properties (#11986)
  golangci-lint: revive: add early-return; fix issues (#12017)
  Implement NewPluginProvider (EVM) (#11995)
  Fix lock file version and minor NPM bumps (#11980)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants