[SECHD-20276] Extend Functions contracts to support ZKSync #15594
Conversation
Static analysis results are availableHey @KodeyThomas, you can view Slither reports in the job summary here or download them as artifact here. |
KodeyThomas
changed the title
KodeyThomas
changed the title
AER Report: CI Coreaer_workflow , commit , Scheduled Run Frequency , Detect Changes , Clean Go Tidy & Generate , Flakeguard Root Project / Get Tests To Run , lint , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Flakeguard Deployment Project , Core Tests (go_core_race_tests) , Core Tests (go_core_fuzz) , Flakeguard Root Project / Run Tests , Flakeguard Root Project / Report , Flakey Test Detection , SonarQube Scan 1. Go toolchain not available:[Aggregate Flakeguard Results]Source of Error:Why: The error occurred because the Go toolchain for version 1.23 on the linux/arm64 architecture is not available. This could be due to an incorrect version specification or a missing toolchain for the specified architecture. Suggested fix: Verify the availability of the Go toolchain for the specified version and architecture. If unavailable, either use a different version or architecture, or ensure the correct toolchain is installed and accessible. |
|
|
||
| uint256 pubdataGasSpent; | ||
| if (success) { | ||
| (, pubdataGasSpent) = abi.decode(returnData, (bytes, uint256)); |
There was a problem hiding this comment.
i understand that we want to limit the return data size to MAX_CALLBACK_RETURN_BYTES. This limit is pretty small? i am not sure if it's a case for revert or we can continue the execution.
if the delegatecall is successful but the return data size is too big, will that make pubdataGasSpent unpredictable?
| /// @dev The exisiting library used within other functions contracts contains opcodes not supported by ZKSync | ||
| library ZKSyncChainSpecificUtil { | ||
| function _getCurrentTxL1GasFees() internal view returns (uint256 l1FeeWei) { | ||
| ISystemContext systemContext = ISystemContext(SYSTEM_CONTEXT); |
There was a problem hiding this comment.
can you make this systemContext a constant or immutable on this lib?
| /// @NOTE: Basis Points are 1/100th of 1%, divide by 10_000 to bring back to original units | ||
|
|
||
| uint256 executionGas = s_config.gasOverheadBeforeCallback + s_config.gasOverheadAfterCallback + callbackGasLimit; | ||
| uint256 l1FeeWei = ZKSyncChainSpecificUtil._getCurrentTxL1GasFees(); |
There was a problem hiding this comment.
i see 2 use cases of _getCurrentTxL1GasFees to get L1 fee. i am not sure if this will give the correct value.
bc we will not know the exact gas cost for updating storage until the entire transaction is executed.
i have this document for auditors and this section explains certain concepts.
traditionally, automation estimates the gas cost by (the upkeep gas limit + L1 gas cost). however, we cannot reliably estimate the L1 gas cost due to the reason above. we have to forward all the gas to gas bound caller to bound the gas usage. this means the max gas usage will simply be the gas limit. so you can see the l1 cost here is 0.
if functions were to follow the similar path and use gas bound caller with callback gas limit, we need to basically provide the gas cost estimation based on the gas limit for the function callback and use 0 for l1 gas cost.
| ISystemContext systemContext = ISystemContext(SYSTEM_CONTEXT); | ||
|
|
||
| // blob_gas_price_on_l1 * gas_per_byte ~= gas_price_on_l2 * l2_gas_PerPubdataByte | ||
| return systemContext.gasPrice() * systemContext.gasPerPubdataByte(); |
There was a problem hiding this comment.
i feel this is not doing what the func sig indicates?
what we have here is L2 gas price * L2 gas per L1 pubdata / byte so it still needs the number of bytes current tx will be updating, in order to correctly represent L1 gas cost in L2 world. however, this brings us back to the problem where it's impossible to know how many L1 bytes will be updated beforehand.
in case you need a reference for these functions, this is the precompiles code
This PR extends the existing Functions contracts to support ZKSync.
The current contracts had to be modified in order to support ZKSync due to the
ChainSpecificUtillibrary containing an opcode ZKSync did not support.In addition to this, I've implemented the L1 fee share logic within the billing logic.
Finally, the GasBoundCaller has been used (in a similar vein to the automation changes) to prevent users from publishing too much pubdata