Skip to content

feat(entropy): Add gas tracking for the callback recovery flow #2666

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 9, 2025
+27 −31
Merged

feat(entropy): Add gas tracking for the callback recovery flow #2666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7d5569
log gas usage
jayantk May 8, 2025
31d3ba6
track gas usage in the recovery flow
jayantk May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 23 additions & 18 deletions target_chains/ethereum/contracts/contracts/entropy/Entropy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,38 +669,43 @@ abstract contract Entropy is IEntropy, EntropyState {
}
} else {
// This case uses the checks-effects-interactions pattern to avoid reentry attacks
emit RevealedWithCallback(
EntropyStructConverter.toV1Request(req),
userRandomNumber,
providerRevelation,
randomNumber
);
emit EntropyEventsV2.Revealed(
provider,
req.requester,
req.sequenceNumber,
randomNumber,
false,
bytes(""),
0, // gas usage not tracked in the old no-gas-limit flow.
bytes("")
);

address callAddress = req.requester;
EntropyStructs.Request memory reqV1 = EntropyStructConverter
.toV1Request(req);
clearRequest(provider, sequenceNumber);
// WARNING: DO NOT USE req BELOW HERE AS ITS CONTENTS HAS BEEN CLEARED
Copy link
Contributor Author

@jayantk jayantk May 8, 2025

Choose a reason for hiding this comment

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

referencing req.requester in the prior version of this code wasn't a good idea. (that field doesn't get cleared, but not good to make assumptions about that)


// Check if the requester is a contract account.
uint len;
address callAddress = req.requester;
assembly {
len := extcodesize(callAddress)
}
uint256 startingGas = gasleft();
if (len != 0) {
IEntropyConsumer(callAddress)._entropyCallback(
sequenceNumber,
provider,
randomNumber
);
}
uint32 gasUsed = SafeCast.toUint32(startingGas - gasleft());

emit RevealedWithCallback(
reqV1,
userRandomNumber,
providerRevelation,
randomNumber
);
emit EntropyEventsV2.Revealed(
provider,
callAddress,
sequenceNumber,
randomNumber,
false,
bytes(""),
gasUsed,
bytes("")
);
}
}

Expand Down
17 changes: 4 additions & 13 deletions target_chains/ethereum/contracts/forge-test/Entropy.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1778,7 +1778,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents, EntropyEventsV2 {
// can never cause a callback to fail because it runs out of gas.
vm.prank(provider1);
random.setDefaultGasLimit(0);

assertCallbackResult(0, 190000, true);
assertCallbackResult(0, 210000, true);
assertCallbackResult(300000, 290000, true);
Expand Down Expand Up @@ -1864,8 +1863,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents, EntropyEventsV2 {
assertEq(callbackFailed, true);
assertEq(callbackErrorCode, bytes(""));

// callback gas usage is approximate and only triggered when the provider has set a gas limit.
// Note: this condition is somewhat janky, but we hit the stack limit so can't put in any more local variables :(
// callback gas usage is approximate
Copy link
Contributor Author

@jayantk jayantk May 8, 2025

Choose a reason for hiding this comment

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

Note that other tests of the Revealed event don't check the gas field because it's too hard to predict up front. Thus, we're using vm.expectEmit to only check the topics not the data.

assertTrue(
random.getProviderInfoV2(provider1).defaultGasLimit == 0 ||
((callbackGasUsage * 90) / 100 < callbackGasUsed)
Expand Down Expand Up @@ -1941,16 +1939,9 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents, EntropyEventsV2 {
);
assertEq(callbackFailed, false);
assertEq(callbackErrorCode, bytes(""));
// callback gas usage is approximate and only triggered when the provider has set a gas limit
// Note: this condition is somewhat janky, but we hit the stack limit so can't put in any more local variables :(
assertTrue(
random.getProviderInfoV2(provider1).defaultGasLimit == 0 ||
((callbackGasUsage * 90) / 100 < callbackGasUsed)
);
assertTrue(
random.getProviderInfoV2(provider1).defaultGasLimit == 0 ||
(callbackGasUsed < (callbackGasUsage * 110) / 100)
);
// callback gas usage is approximate
assertTrue((callbackGasUsage * 90) / 100 < callbackGasUsed);
assertTrue(callbackGasUsed < (callbackGasUsage * 110) / 100);
assertEq(extraArgs, bytes(""));

// Verify request is cleared after successful callback
Expand Down
Loading