Skip to content

fix: updateAnalytics() takes gasValueUsed arg #412

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
Aug 22, 2024
Merged

fix: updateAnalytics() takes gasValueUsed arg #412

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
c1c3384
fix: `updateAnalytics()` takes `gasValueUsed` arg
BenSparksCode Aug 21, 2024
c9af56c
Merge branch 'main' into fix-analytics-overflow
BenSparksCode Aug 21, 2024
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
28 changes: 18 additions & 10 deletions src/contracts/atlas/GasAccounting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,13 @@ abstract contract GasAccounting is SafetyLocks {
/// increase transient solver deposits.
/// @param owner The address of the owner from whom AtlETH is taken.
/// @param amount The amount of AtlETH to be taken.
/// @param gasUsed The amount of gas used in the SolverOperation.
/// @param gasValueUsed The ETH value of gas used in the SolverOperation.
/// @param solverWon A boolean indicating whether the solver won the bid.
/// @return deficit The amount of AtlETH that was not repaid, if any.
function _assign(
address owner,
uint256 amount,
uint256 gasUsed,
uint256 gasValueUsed,
bool solverWon
)
internal
Expand Down Expand Up @@ -222,8 +222,8 @@ abstract contract GasAccounting is SafetyLocks {
_aData.bonded -= _amt;
}

// Update analytics (auctionWins, auctionFails, totalGasUsed) and lastAccessedBlock
_updateAnalytics(_aData, solverWon && deficit == 0, gasUsed);
// Update analytics (auctionWins, auctionFails, totalGasValueUsed) and lastAccessedBlock
_updateAnalytics(_aData, solverWon && deficit == 0, gasValueUsed);
_aData.lastAccessedBlock = uint32(block.number);

// Persist changes in the _aData memory struct back to storage
Expand All @@ -236,7 +236,8 @@ abstract contract GasAccounting is SafetyLocks {
/// @notice Increases the owner's bonded balance by the specified amount.
/// @param owner The address of the owner whose bonded balance will be increased.
/// @param amount The amount by which to increase the owner's bonded balance.
function _credit(address owner, uint256 amount, uint256 gasUsed) internal {
/// @param gasValueUsed The ETH value of gas used in the SolverOperation.
function _credit(address owner, uint256 amount, uint256 gasValueUsed) internal {
uint112 _amt = SafeCast.toUint112(amount);

EscrowAccountAccessData memory _aData = S_accessData[owner];
Expand All @@ -246,8 +247,8 @@ abstract contract GasAccounting is SafetyLocks {

S_bondedTotalSupply += amount;

// Update analytics (auctionWins, auctionFails, totalGasUsed)
_updateAnalytics(_aData, true, gasUsed);
// Update analytics (auctionWins, auctionFails, totalGasValueUsed)
_updateAnalytics(_aData, true, gasValueUsed);

// Persist changes in the _aData memory struct back to storage
S_accessData[owner] = _aData;
Expand Down Expand Up @@ -437,8 +438,15 @@ abstract contract GasAccounting is SafetyLocks {
/// @dev This function is only ever called in the context of bidFind = false so no risk of doublecounting changes.
/// @param aData The Solver's EscrowAccountAccessData struct to update.
/// @param auctionWon A boolean indicating whether the solver's solverOp won the auction.
/// @param gasUsed The amount of gas used by the solverOp.
function _updateAnalytics(EscrowAccountAccessData memory aData, bool auctionWon, uint256 gasUsed) internal view {
/// @param gasValueUsed The ETH value of gas used by the solverOp. Should be calculated as gasUsed * tx.gasprice.
function _updateAnalytics(
EscrowAccountAccessData memory aData,
bool auctionWon,
uint256 gasValueUsed
)
internal
pure
{
if (auctionWon) {
unchecked {
++aData.auctionWins;
Expand All @@ -450,7 +458,7 @@ abstract contract GasAccounting is SafetyLocks {
}

// Track total ETH value of gas spent by solver in metacalls. Measured in gwei (1e9 digits truncated).
aData.totalGasValueUsed += uint64(gasUsed * tx.gasprice / _GAS_VALUE_DECIMALS_TO_DROP);
aData.totalGasValueUsed += SafeCast.toUint64(gasValueUsed / _GAS_VALUE_DECIMALS_TO_DROP);
}

/// @notice Calculates the gas cost of the calldata used to execute a SolverOperation.
Expand Down
46 changes: 30 additions & 16 deletions test/GasAccounting.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ contract MockGasAccounting is TestAtlas, BaseTest {
// Expose access to internal functions for testing //
/////////////////////////////////////////////////////////

function assign(address owner, uint256 value, bool solverWon) external returns (uint256) {
return _assign(owner, value, value, solverWon);
function assign(address owner, uint256 amount, uint256 gasValueUsed, bool solverWon) external returns (uint256) {
return _assign(owner, amount, gasValueUsed, solverWon);
}

function credit(address owner, uint256 value) external {
_credit(owner, value, value);
function credit(address owner, uint256 amount, uint256 gasValueUsed) external {
_credit(owner, amount, gasValueUsed);
}

function handleSolverAccounting(
Expand Down Expand Up @@ -792,7 +792,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {
uint256 bondedTotalSupplyBefore = mockGasAccounting.bondedTotalSupply();
uint256 depositsBefore = mockGasAccounting.getDeposits();

uint256 deficit = mockGasAccounting.assign(solverOp.from, assignedAmount, true);
uint256 deficit = mockGasAccounting.assign(solverOp.from, assignedAmount, assignedAmount, true);
assertEq(deficit, 0, "Deficit should be 0");

(, uint32 lastAccessedBlock,,,) = mockGasAccounting.accessData(solverOp.from);
Expand All @@ -818,7 +818,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {
uint256 depositsBefore = mockGasAccounting.getDeposits();

// Call the assign function and capture the deficit
uint256 deficit = mockGasAccounting.assign(solverOp.from, assignedAmount, true);
uint256 deficit = mockGasAccounting.assign(solverOp.from, assignedAmount, assignedAmount, true);
assertEq(deficit, 0, "Deficit should be 0");

// Retrieve and check the updated access data
Expand Down Expand Up @@ -850,7 +850,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {

uint256 bondedTotalSupplyBefore = mockGasAccounting.bondedTotalSupply();
uint256 depositsBefore = mockGasAccounting.getDeposits();
uint256 deficit = mockGasAccounting.assign(solverOp.from, assignedAmount, true);
uint256 deficit = mockGasAccounting.assign(solverOp.from, assignedAmount, assignedAmount, true);
assertEq(deficit, assignedAmount - (unbondingAmount + bondedAmount));
(, uint32 lastAccessedBlock,,,) = mockGasAccounting.accessData(solverOp.from);
assertEq(lastAccessedBlock, uint32(block.number));
Expand All @@ -862,10 +862,12 @@ contract GasAccountingTest is AtlasConstants, BaseTest {
}

function test_assign_reputationAnalytics() public {
// NOTE: the `amount` and `gasValueUsed` params for `_assign()` should be measured in ETH value. I.e. they should be calculated as `gasUsed * tx.gasprice`.
uint256 startGasPrice = 2e9;
uint256 endGasPrice = 4e9;

uint256 assignedAmount = 1_234_567;
uint256 gasUsedAmount = 1_234_567;
uint256 assignedAmount;
uint24 auctionWins;
uint24 auctionFails;
uint64 totalGasValueUsed;
Expand All @@ -879,20 +881,32 @@ contract GasAccountingTest is AtlasConstants, BaseTest {

vm.txGasPrice(startGasPrice); // Set gas price to 2e9
assertEq(tx.gasprice, startGasPrice, "tx.gasprice should be 2e9");
mockGasAccounting.assign(solverOp.from, assignedAmount, true);
assignedAmount = gasUsedAmount * tx.gasprice;
mockGasAccounting.assign({
owner: solverOp.from,
amount: assignedAmount,
gasValueUsed: assignedAmount,
solverWon: true
});

(,, auctionWins, auctionFails, totalGasValueUsed) = mockGasAccounting.accessData(solverOp.from);
expectedTotalGasValueUsed = assignedAmount * startGasPrice / ONE_GWEI;
expectedTotalGasValueUsed = assignedAmount / ONE_GWEI;
assertEq(auctionWins, 1, "auctionWins should be incremented by 1");
assertEq(auctionFails, 0, "auctionFails should remain at 0");
assertEq(totalGasValueUsed, expectedTotalGasValueUsed, "totalGasValueUsed not as expected");

vm.txGasPrice(endGasPrice); // Set gas price to 4e9
assertEq(tx.gasprice, endGasPrice, "tx.gasprice should be 4e9");
mockGasAccounting.assign(solverOp.from, assignedAmount, false);
assignedAmount = gasUsedAmount * tx.gasprice;
mockGasAccounting.assign({
owner: solverOp.from,
amount: assignedAmount,
gasValueUsed: assignedAmount,
solverWon: false
});

(,, auctionWins, auctionFails, totalGasValueUsed) = mockGasAccounting.accessData(solverOp.from);
expectedTotalGasValueUsed += assignedAmount * endGasPrice / ONE_GWEI;
expectedTotalGasValueUsed += assignedAmount / ONE_GWEI;
assertEq(auctionWins, 1, "auctionWins should remain at 1");
assertEq(auctionFails, 1, "auctionFails should be incremented by 1");
assertEq(totalGasValueUsed, expectedTotalGasValueUsed, "totalGasValueUsed not as expected");
Expand All @@ -907,7 +921,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {
uint256 depositsBefore = mockGasAccounting.getDeposits();
(uint112 unbondingBefore,) = mockGasAccounting._balanceOf(solverOp.from);
vm.expectRevert(AtlasErrors.ValueTooLarge.selector);
mockGasAccounting.assign(solverOp.from, assignedAmount, true);
mockGasAccounting.assign(solverOp.from, assignedAmount, assignedAmount, true);

// Check assign reverted with overflow, and accounting values did not change
assertEq(mockGasAccounting.bondedTotalSupply(), bondedTotalSupplyBefore);
Expand All @@ -926,7 +940,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {
(, lastAccessedBlock,,,) = mockGasAccounting.accessData(solverOp.from);
assertEq(lastAccessedBlock, 0);

mockGasAccounting.credit(solverOp.from, creditedAmount);
mockGasAccounting.credit(solverOp.from, creditedAmount, creditedAmount);

(, lastAccessedBlock,,,) = mockGasAccounting.accessData(solverOp.from);
(uint112 bondedAfter,,,,) = mockGasAccounting.accessData(solverOp.from);
Expand All @@ -939,7 +953,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {
// Testing uint112 boundary values for casting from uint256 to uint112 in _credit()
uint256 overflowAmount = uint256(type(uint112).max) + 1;
vm.expectRevert(abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 112, overflowAmount));
mockGasAccounting.credit(solverOp.from, overflowAmount);
mockGasAccounting.credit(solverOp.from, overflowAmount, overflowAmount);
}

function test_handleSolverAccounting_solverNotResponsible() public {
Expand All @@ -962,7 +976,7 @@ contract GasAccountingTest is AtlasConstants, BaseTest {

uint256 expectedWriteoffs = initialWriteoffs + AccountingMath.withAtlasAndBundlerSurcharges(gasUsed);
// Verify writeoffs have increased
assertEq(mockGasAccounting.getWriteoffs(), expectedWriteoffs, "Writeoffs mismatch");
assertApproxEqRel(mockGasAccounting.getWriteoffs(), expectedWriteoffs, 1e15, "Writeoffs not within 0.1% error margin");
}

function test_handleSolverAccounting_solverResponsible() public {
Expand Down