Implement new gas tracking contracts #874

ibremseth · 2025-10-23T18:15:49Z

What does this PR do?

Summary:
Updates upgradable contracts to use an external contract for tracking gas called GasMeter. Any appchain can call this contract directly with the function they want tracked (through the fallback function on GasMeter) or explicitly with .meterCall(...). GasMeter will then call back to the msg.sender with the passed in calldata (or exact calldata if through the fallback function) while keeping track of how much gas it uses and stores it per epoch.

How can this PR be tested?

forge test

ibremseth · 2025-10-23T18:17:50Z

synd-contracts/src/staking/GasMeter.sol

+    fallback() external {
+        meterCall(msg.data);
+    }


This is not needed, but I kinda think its a cool way for SeqChains contracts to call GasMeter for any of its functions! If people hate it, happy to drop

I like the MyContract(gasMeterAddress).fooBar syntax, but think it's not worth it since using a fallback function can lead to function name collisions and unexpected behavior if the gas aggregator and sequencing contracts define a function with the same name and parameters. This is especially dangerous if the gas aggregator is updated later on to add new functions to it - the contract upgrade can break existing sequencing contracts that will end up calling the newly added function instead of fallback().

Instead of abi.encodeWithSelector, you can use abi.encodeCall to get the same type-checking benefits as the fallback approach, even if the syntax is slightly uglier.

synd-contracts/src/SyndicateSequencingChain.sol

ibremseth · 2025-10-23T18:20:42Z

synd-contracts/src/SyndicateSequencingChain.sol

    uint256 version;
+    /// @notice The address of the GasMeter contract
+    /// @dev This is set during initialization and never changes
+    address gasMeter;


Not sure if it makes sense to move this into a different struct, since its not related to the CORE attributes of the chain and we could probably drop it once emissions are done?

I would make this an immutable variable instead to save gas - if we really want to modify this post-deployment, we can just redeploy the implementation contract with a new gasMeter value and upgrade the proxy to the new implementation.

tsite

Amazing work - I love this design pattern & think it's a really clean and extensible way to track gas and run pre/post tx hooks! I left a couple small comments/questions, but it looks great overall

tsite · 2025-10-23T19:16:05Z

synd-contracts/src/staking/GasMeter.sol

+    fallback() external {
+        meterCall(msg.data);
+    }


I like the MyContract(gasMeterAddress).fooBar syntax, but think it's not worth it since using a fallback function can lead to function name collisions and unexpected behavior if the gas aggregator and sequencing contracts define a function with the same name and parameters. This is especially dangerous if the gas aggregator is updated later on to add new functions to it - the contract upgrade can break existing sequencing contracts that will end up calling the newly added function instead of fallback().

Instead of abi.encodeWithSelector, you can use abi.encodeCall to get the same type-checking benefits as the fallback approach, even if the syntax is slightly uglier.

synd-contracts/src/SyndicateSequencingChain.sol

synd-contracts/src/factory/SyndicateFactory.sol

synd-contracts/src/SyndicateSequencingChain.sol

tsite · 2025-10-23T19:33:45Z

synd-contracts/src/SyndicateSequencingChain.sol

    uint256 version;
+    /// @notice The address of the GasMeter contract
+    /// @dev This is set during initialization and never changes
+    address gasMeter;


I would make this an immutable variable instead to save gas - if we really want to modify this post-deployment, we can just redeploy the implementation contract with a new gasMeter value and upgrade the proxy to the new implementation.

synd-contracts/src/SyndicateSequencingChain.sol

tsite · 2025-10-23T19:51:05Z

synd-contracts/src/staking/GasMeter.sol

+        uint256 startGas = gasleft();
+        (bool success, bytes memory result) = address(msg.sender).call(meteredCall);
+        if (!success) {
+            revert(string(result));


think we should use
assembly { revert(result, mload(result)) }
instead to perfectly forward the revert reason. I think revert(string) will double abi-encode the error since result should already be abi-encoded, but not 100% sure if that's how it works.

I tried updating to that assembly code, but a bunch of tests started failing due to lack of revert reasons, so I think the current way is correct

ah yep it should be assembly { revert(add(result, 32), mload(result)) } instead. I guess revert(string(result)) works too though, apparently the compiler is smart enough to figure out that it's already encoded and doesn't double encode the error.

I did some digging and it turns out the error is being double abi encoded, just the foundry vm.expectRevert function automatically strips the "Error(string)" selector that revert(string) adds to errors if it is present. So I think we should definitely use the assembly code to prevent that.

tsite · 2025-10-23T19:52:21Z

synd-contracts/src/staking/GasMeter.sol

+        if (!success) {
+            revert(string(result));
+        }
+        _getGasMeterStorage().gasUsed[getCurrentEpoch()][msg.sender] += startGas - gasleft();


we may want to add
assembly { return(result, mload(result)) }
to perfectly forward the return value. I doubt any code will use this return value, but it's probably worth forwarding it regardless just in case.

also think this should be (startGas - gasleft()) * tx.gasprice instead

the assembly should be
assembly { return(add(result, 32), mload(result)) }
instead

tsite · 2025-10-23T19:57:53Z

synd-contracts/src/SyndicateSequencingChain.sol

-
-    /// @notice Disable gas tracking if needed
-    /// @dev Only callable by the contract owner
-    function disableGasTracking() external onlyOwner {


one nice property of this design is the chain owner can disable gas tracking at any point in time by upgrading the sequencing contract to remove the calls to the gas meter

Agree. It would be nice to keep it.

synd-contracts/src/factory/SyndicateFactory.sol

synd-contracts/src/SyndicateSequencingChain.sol

jorgemmsilva · 2025-10-24T09:10:58Z

synd-contracts/src/staking/GasMeter.sol

+    /// @dev Allows for contracts to call the gas meter address like they would call a function on their own contract to track gas used
+    ///      Allows for usage like: MyContract(gasMeterAddress).fooBar(arg1, arg2, arg3);
+    ///      Instead of: gasMeterAddress.meterCall(abi.encodeWithSelector(MyContract.fooBar.selector, arg1, arg2, arg3));
+    fallback() external {


Think we need some sort of reentrancy guard, to prevent a contract doing:

gas meter -> myImpl -> gas meter -> myImpl -> ...

good catch. alternatively, I think we can allow reentrancy and change the code to something like

uint256 startGas = gasleft() + _getGasMeterStorage().gasUsed[getCurrentEpoch()][msg.sender]; address(msg.sender).call(meteredCall); _getGasMeterStorage().gasUsed[getCurrentEpoch()][msg.sender] = startGas - gasleft();

instead

jorgemmsilva · 2025-10-24T09:14:41Z

synd-contracts/src/staking/GasMeter.sol

+    /// @notice Meter a call and track the gas used
+    /// @dev Meters the gas used for a call and tracks it in the gas used mapping
+    /// @param meteredCall The call to track gas for
+    function meterCall(bytes calldata meteredCall) public {


should we make this internal, so all calls come via the fallback method?

jorgemmsilva

this solution is really elegant and a step up from what we had before, great thinking! 🧠

…r epoch

tsite

lgtm % immutable gasMeter variable, return value forwarding, and ReentrancyGuardTransientUpgradeable

tsite · 2025-10-27T17:02:53Z

synd-contracts/src/staking/GasMeter.sol

+import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import {EpochTracker} from "./EpochTracker.sol";
+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";


can we use ReentrancyGuardTransientUpgradeable instead? that uses transient instead of regular storage and should be a lot cheaper in terms of gas cost

tsite · 2025-10-27T18:04:56Z

synd-contracts/src/factory/SyndicateFactory.sol

+            abi.encodeCall(SyndicateSequencingChain.initialize, (admin, address(permissionModule), chainId));
        (bool upgradeSuccess,) = sequencingChain.call(
-            abi.encodeWithSignature("upgradeToAndCall(address,bytes)", $.syndicateChainImpl, initData)
+            abi.encodeWithSignature("upgradeToAndCall(address,bytes)", syndicateChainImpl, initData)


nit: I think abi.encodeCall(UUPSUpgradeable.upgradeToAndCall, ...) should work here for explicit type checking

jorgemmsilva · 2025-10-28T15:59:24Z

synd-contracts/src/staking/GasMeter.sol

+            }
+        }
+
+        uint256 gasPrice = tx.gasprice == 0 ? 1 : tx.gasprice;


We should probably leave a comment that explains why this workaround is needed for gas estimation.
The necessity of 0 ? 1 might be lost to the reader in the future

ibremseth commented Oct 23, 2025

View reviewed changes

synd-contracts/src/SyndicateSequencingChain.sol Outdated Show resolved Hide resolved

ibremseth commented Oct 23, 2025

View reviewed changes

ibremseth marked this pull request as ready for review October 23, 2025 18:24

tsite self-requested a review October 23, 2025 20:06

tsite requested changes Oct 23, 2025

View reviewed changes

tsite reviewed Oct 23, 2025

View reviewed changes

synd-contracts/src/SyndicateSequencingChain.sol Outdated Show resolved Hide resolved

tsite reviewed Oct 23, 2025

View reviewed changes

synd-contracts/src/SyndicateSequencingChain.sol Outdated Show resolved Hide resolved

jorgemmsilva reviewed Oct 24, 2025

View reviewed changes

tsite requested a review from gustavoguimaraes October 24, 2025 18:04

ibremseth added 5 commits October 27, 2025 09:07

New gas metering contract used to track how much gas appchains use pe…

5a77202

…r epoch

fix test

5ec389d

PR updates and contract bindings

357bb96

assembly

1f57d39

fix merge mistake

c61f2d8

ibremseth force-pushed the new-gas-tracker branch from 0ccbde1 to c61f2d8 Compare October 27, 2025 13:43

ibremseth added 7 commits October 27, 2025 09:54

fix certora

0f2e23e

certora fix attempt #2

12a7eed

skip flaky fork test and fix certora syntax

b691857

fmt, cerotra fix, and cli fix

d8c923e

one more certora fix

3b2f000

fix cli

5e5220b

fix cli

635efcc

tsite self-requested a review October 27, 2025 17:47

tsite approved these changes Oct 27, 2025

View reviewed changes

ibremseth added 3 commits October 27, 2025 16:00

fix tests

0e89fdb

fmt

5233da7

fmt

341cfc1

jorgemmsilva reviewed Oct 28, 2025

View reviewed changes

Uh oh!

Implement new gas tracking contracts #874

Are you sure you want to change the base?

Implement new gas tracking contracts #874

Uh oh!

Conversation

ibremseth commented Oct 23, 2025

What does this PR do?

How can this PR be tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsite Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsite Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsite Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgemmsilva left a comment

Choose a reason for hiding this comment

Uh oh!

tsite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsite Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

tsite Oct 25, 2025 •

edited

Loading

tsite Oct 23, 2025 •

edited

Loading

tsite Oct 24, 2025 •

edited

Loading

tsite Oct 27, 2025 •

edited

Loading