Diamond Proxy pattern based smart contracts by udityadav-supraoracles · Pull Request #16 · Entropy-Foundation/revm

udityadav-supraoracles · 2026-02-17T06:21:48Z

This PR includes changes to accommodate use of Diamond Proxy pattern for EVM Automation Registry smart contracts.

-includes:
facets: ConfigFacet. RegistryFacet and CoreFacet.
DiamondInit: This contract is used to initialise the state of Diamond, its not added as facet. So, we don't have a flag to check for initialisation. Once Diamond is deployed, this contract is supposed to be called as part of diamondCut.

…mation_registry

udityadav-supraoracles · 2026-02-19T11:50:20Z

Currently the AppStorage have inner structs which doesn't allow the inner structs to be expanded later. Either we can have all variables in AppStorage struct or have a mapping pointing to inner struct as structs pointing by mapping are expandable.

Referring to: https://eip2535diamonds.substack.com/p/diamond-upgrades
Do not put structs directly in structs unless you don’t plan on ever adding more state variables to the inner structs. You won't be able to add new state variables to inner structs in upgrades without overwriting existing state variables.

Please let me know your opinion Dr @sjoshisupra @aregng.

sjoshisupra · 2026-02-20T10:13:56Z

solidity/supra_contracts/src/facets/ConfigFacet.sol

+    function setVmSigner(address _vmSigner) external {
+        LibDiamond.enforceIsContractOwner();
+
+        LibUtils.validateAddress(_vmSigner);
+
+        address oldVmSigner = s.vmSigner;
+        s.vmSigner = _vmSigner;
+
+        emit VmSignerUpdated(oldVmSigner, _vmSigner);
+    }


@udityadav-supraoracles, @aregng, I do not foresee any circumstances for VM_SIGNER to change since this is hard coded in rust layer. Is this method only to initialize after contract deployment or do we foresee this to be useful? If not, perhaps we should not have this API (if this is initialized with appropriate VM_SIGNER address at the time of deployment)

The VM_SIGNER and ERC20Supra both are initialised at the time of deployment. We can remove their respective API's to update the addresses if we do not plan to change them later. Please let me know your opinion @aregng .

VM_SIGNER value will not change for sure.
Regarding ERC20Supra: currently we are deploying it as a single contract as non upgradable. If we make it upgradable for any potential upgrades in future and deploy it as a proxy contract, then I think there will not be a need to have the instance updated in scope of registry as well.

The ERC20Supra is going to be upgradable, considering that I'm removing APIs to update ERC20Supra and VM_SIGNER.

sjoshisupra · 2026-02-20T10:16:45Z

solidity/supra_contracts/src/facets/ConfigFacet.sol

+
+    /// @notice Function to update the ERC20Supra address.
+    /// @param _erc20Supra New address for ERC20Supra.
+    function setErc20Supra(address _erc20Supra) external {


@udityadav-supraoracles, is this to update ERC 20 contract address? Or to update the address of the registry itself where it stores ERC20 tokens collected from fee? It is not clear from the comments.
If it is the former, why does registry contract need to know ERC20 contract address?

This function is meant to update the address of ERC20Supra contract in registry, as the registry holds ERC20Supra address. The registry needs to know ERC20Supra's address in order to call transferFrom on it while charging fees.

Regarding fee collection, fee charged in terms of ERC20Supra tokens is stored at the registry's address(ERC20 standard contract inherited by ERC20Supra maintains mapping _balances where it stores the registry's balance).

sjoshisupra · 2026-02-20T10:23:25Z

solidity/supra_contracts/src/facets/ConfigFacet.sol

+        uint256 balance = IERC20(s.erc20Supra).balanceOf(address(this));
+
+        if (balance < _amount) { revert InsufficientBalance(); }
+        if (balance - _amount < s.registryState.cycleLockedFees + s.registryState.totalDepositedAutomationFees) { revert RequestExceedsLockedBalance(); }


@udityadav-supraoracles, something seems off. I believe we want to withdraw from s.registryState.totalDepostedAutomationFees, meaning that shouldn't the expression be
balance - _amount < s.registryState.cycleLockedFees? does totalDepositedAutomationFees refer to fees or does it refer to the caution that we take to deter DOS via cancellation?

The registry contract holds balances for cycleLockedFees and totalDepositedAutomationFees where totalDepositedAutomationFees refers to automation fee deposited by the users for their tasks during registration.

Only the amount which's been unlocked can be withdrawn. For example, lets say the contract holds 100 ERC20Supra token which comprises of cycleLockedFees and totalDepositedAutomationFees out of which 20 has been unlocked and 80 are still locked as cycleLockedFees and totalDepositedAutomationFees. This condition ensures that we are not reducing the locked cycleLockedFees and totalDepositedAutomationFees.

sjoshisupra · 2026-02-20T10:24:59Z

solidity/supra_contracts/src/facets/ConfigFacet.sol

+        if (s.registryState.gasCommittedForNextCycle > _registryMaxGasCap) { revert UnacceptableRegistryMaxGasCap(); }
+        if (s.registryState.sysGasCommittedForNextCycle > _sysRegistryMaxGasCap) { revert UnacceptableSysRegistryMaxGasCap(); }


@udityadav-supraoracles, shouldn't these two checks also be part of LibCommon.validateConfigParameters method? This seems like validation checks only.

It couldn't because LibCommon.validateConfigParameters is used while initialisation and updating the buffer. But in case of updating buffer the above mentioned validation is additional which is not required during initialisation.

sjoshisupra · 2026-02-20T10:27:47Z

solidity/supra_contracts/src/facets/CoreFacet.sol

+    /// @param _taskIndexes Array of task index to be processed.
+    function processTasks(uint64 _cycleIndex, uint64[] memory _taskIndexes) external {
+        // Check caller is VM Signer
+        if (msg.sender != s.vmSigner) { revert CallerNotVmSigner(); }


@udityadav-supraoracles, it is better to have a common method enforceIsVmSigner similar to enforceIsContractOwner.

solidity/supra_contracts/src/libraries/LibAccounting.sol

solidity/supra_contracts/src/libraries/LibCommon.sol

sjoshisupra · 2026-02-20T11:12:45Z

solidity/supra_contracts/src/libraries/LibCore.sol

+    /// @return intermediateState Returns the intermediate state.
+    function dropOrChargeTasks(
+        uint64[] memory _taskIndexes
+    ) private returns (LibCommon.IntermediateStateOfCycleChange memory intermediateState) {


would this be private? I believe this need to be called by VM_SIGNER right? In fact, multiple places I believe methods are marked as private which may not work with evm and they need to be restricted to be called by VM_SIGNER only. @udityadav-supraoracles @aregng

It is called from onCycleTransition defined as internal in the same library which furthur gets called by VM_SIGNER facing function processTasks. So, there's no issue with this function being marked as private as its not directly called by VM_SIGNER.

VM_SIGNER only calls monitorCycleEnd and processTasks which're marked as external and other helper functions are marked as internal or private.

sjoshisupra · 2026-02-24T10:57:43Z

Currently the AppStorage have inner structs which doesn't allow the inner structs to be expanded later. Either we can have all variables in AppStorage struct or have a mapping pointing to inner struct as structs pointing by mapping are expandable.

Referring to: https://eip2535diamonds.substack.com/p/diamond-upgrades Do not put structs directly in structs unless you don’t plan on ever adding more state variables to the inner structs. You won't be able to add new state variables to inner structs in upgrades without overwriting existing state variables.

Please let me know your opinion Dr @sjoshisupra @aregng.

Hi @udityadav-supraoracles , in that case we should use the guidelines and avoid nested structs.

sjoshisupra · 2026-02-27T12:25:48Z

solidity/supra_contracts/src/libraries/LibAccounting.sol

+        for (uint8 i = 0; i < _exponent; i++) {
+            resultScaled = (resultScaled * baseScaled) / DECIMAL;
+        }


Suggested change

for (uint8 i = 0; i < _exponent; i++) {

resultScaled = (resultScaled * baseScaled) / DECIMAL;

}

while (_exponent > 0) {

if (_exponent & 0x1) {

resultScaled = (resultScaled * baseScaled) / DECIMAL;

}

else {

baseScaled = baseScaled << 1;

}

_exponent = _exponent >> 1;

}

I think this is logarithmic in the value of the exponent whereas at present linear algorithm is used.

Yeah, updated with the following:

while (_exponent > 0) { if ((_exponent & 1) != 0) { resultScaled = (resultScaled * baseScaled) / DECIMAL; } _exponent >>= 1; if (_exponent > 0) { baseScaled = (baseScaled * baseScaled) / DECIMAL; } }

sjoshisupra · 2026-02-27T12:34:24Z

solidity/supra_contracts/src/libraries/LibCommon.sol

+    /// @param _taskIndex Task index to check if a task exists against it.
+    function ifTaskExists(uint64 _taskIndex) internal view returns (bool) {
+        RegistryState storage registryState = LibAppStorage.registryState();
+        return registryState.tasks[_taskIndex].owner != address(0) && registryState.taskIdList.contains(_taskIndex);


@udityadav-supraoracles, address(0) does not indicate a contract, right?

No, address(0) is default value for address type.

sjoshisupra · 2026-02-27T12:36:15Z

solidity/supra_contracts/src/libraries/LibCommon.sol

+
+        delete registryState.tasks[_taskIndex];
+        require(registryState.taskIdList.remove(_taskIndex), TaskIndexNotFound());
+        require(registryState.userTasks[_owner].remove(_taskIndex), TaskIndexNotFound());


If this is a system task, won't it violate this condition? @udityadav-supraoracles

Actually registryState.userTasks keeps track of address => registered tasks. The name userTasks is confusing, it keeps track of registered tasks by an address which comprises of both UST as well as GST. Updated the name to addressToTasks.

sjoshisupra · 2026-02-27T12:38:14Z

solidity/supra_contracts/src/libraries/LibDiamond.sol

+
+library LibDiamond {
+    // 32 bytes keccak hash of a string to use as a diamond storage location.
+    bytes32 constant DIAMOND_STORAGE_POSITION = keccak256("diamond.standard.diamond.storage");


Shall we modify the string passed to keccak256? It looks like this came from template and to ensure the storage at random location, we should use a unique string. @udityadav-supraoracles

Yes, this is from the standard Diamond implementation. Existing string "diamond.standard.diamond.storage" also creates a unique storage slot for our Diamond contract.

solidity/supra_contracts/src/libraries/LibDiamondUtils.sol

-Resolved PR comments

- removed setErc20Supra, setVmSigner

aregng · 2026-03-26T18:58:29Z

solidity/supra_contracts/src/ERC20SupraHandler.sol

+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
+
+contract ERC20SupraHandler is OwnableUpgradeable, UUPSUpgradeable {


If my understanding is correct, then user will be using ERC20SupraHandler instead of ERC20Supra, so taking this into account I think ERC20SupraHandler should provide means to check balance, and change the allowance (increase or decrease) without minting. Otherwise user should deal with 2 contracts which will be inconvenient.

solidity/supra_contracts/src/libraries/LibAccounting.sol

sjoshisupra · 2026-03-27T05:23:05Z

solidity/supra_contracts/src/libraries/LibDiamondUtils.sol

-            automationBaseFeeWeiPerSec: 0.001 ether,
-            flatRegistrationFeeWei: 0.002 ether,
+            taskDurationCapSecs: 3600 * 24 * 7,
+            registryMaxGasCap: 1_000_000,


This requires some more thoughts. What is the max gas at chain level for EVM? Since EVM gas cost table has not changed, I believe 1M for entire registry it too low, given that Ethereum allows 18M max gas for a single tx.

sjoshisupra · 2026-03-27T05:24:24Z

solidity/supra_contracts/src/libraries/LibDiamondUtils.sol

+            taskCapacity: 400,
+            cycleDurationSecs: 1200,
+            sysTaskDurationCapSecs: 3600 * 24 * 180,
+            sysRegistryMaxGasCap: 1_000_000,


same here ,this could be too low for EVM

sjoshisupra · 2026-03-27T05:28:52Z

solidity/supra_contracts/src/ERC20Supra.sol

+        uint256 _amount
+    ) external {
+        if (msg.sender != erc20SupraHandler) revert UnauthorizedCaller();
+        _approve(_owner, _spender, _amount);


Does _approve not have any access control? Shouldn't only the owner of the account be able to give approval? What use case are we covering by giving ERC20SupraHandler the wildcard authority?

As discussed earlier we need an API nativeToErc20SupraWithAllowance using which user can convert native to ERC20Supra while setting allowance to Automation Registry in a single transaction.

_approve is an internal function it can only be called in ERC20Supra and not ERC20SupraHandler, so I had to expose an endpoint using which nativeToErc20SupraWithAllowance in ERC20SupraHandler can set allowance during conversion.

solidity/supra_contracts/src/ERC20Supra.sol

sjoshisupra · 2026-03-27T05:34:43Z

solidity/supra_contracts/src/ERC20SupraHandler.sol

+        if (msg.value == 0) revert InvalidAmount();
+        IERC20Supra(erc20Supra).mint(msg.sender, msg.value);
+
+        emit NativeToERC20Supra(msg.sender, msg.value);


Shouldn't we deduct native token from the sender?

The function is marked as payable, therefore user is supposed to send the native tokens along with the tx which gets deposited in the contract.

sjoshisupra · 2026-03-27T05:35:35Z

solidity/supra_contracts/src/ERC20SupraHandler.sol

+    /// @notice Deposits native tokens, mints ERC20Supra tokens 1:1, and sets an allowance for a spender.
+    /// @param _spender The address whose allowance will be set.    
+    /// @param _allowanceAmount The new allowance to set for the spender.
+    function nativeToErc20SupraWithAllowance(address _spender, uint256 _allowanceAmount) external payable {


Not sure why we need this. This goes against separation of concerns.

We needed an API nativeToErc20SupraWithAllowance using which user can convert native to ERC20Supra while setting allowance to Automation Registry in a single transaction.

We can remove this taking into account separation of concerns but then user would've to execute two transactions.

As a user I would say this is a convenience that I would like to have.

sjoshisupra · 2026-03-27T05:36:43Z

solidity/supra_contracts/src/ERC20SupraHandler.sol

+        if (_allowanceAmount == 0) revert InvalidAllowance();
+
+        IERC20Supra(erc20Supra).mint(msg.sender, msg.value);
+        IERC20Supra(erc20Supra).approveFor(msg.sender, _spender, _allowanceAmount);
+
+        emit NativeToERC20SupraWithAllowance(msg.sender, msg.value, _spender, _allowanceAmount);


again, native token should be deducted from sender, what does validateAddress() do here?

The function is marked as payable, therefore user is supposed to send the native tokens along with the tx which gets deposited in the contract.

validateAddress is used to validate the address of spender, the address which is granted allowance to spend. This could be Automation Registry address.

sjoshisupra · 2026-03-27T05:39:56Z

solidity/supra_contracts/src/ERC20SupraHandler.sol

+        (bool sent, ) = payable(msg.sender).call{value: _amount}("");
+        if (!sent) revert TransferFailed();


check that this handler contract has sufficient native balance for transfer. Also, shouldn't there be a direct transfer instead of an external call and then checking status? What's the difference between this method and doing direct native transfer?

Unlike send and transfer, which forward only 2300 gas and can fail with contract recipients, call forwards all remaining gas and is more reliable. It also returns a bool so we can explicitly check success and revert if needed. Using call is the recommended approach.

solidity/supra_contracts/src/ERC20SupraHandler.sol

refactored to use diamond proxy

2dff29c

udityadav-supraoracles requested review from aregng and sjoshisupra February 17, 2026 06:21

udityadav-supraoracles self-assigned this Feb 17, 2026

udityadav-supraoracles added 4 commits February 17, 2026 14:40

minor fixes

1c0b743

refactored code

cb855fe

updated scripts

3992962

refactored

3f6f2df

udityadav-supraoracles marked this pull request as ready for review February 19, 2026 08:43

udityadav-supraoracles added 2 commits February 19, 2026 15:35

fix to remove task if insufficient allowance while bookkeeping

cc73404

Merge branch 'feature/evm_automation' into feature/diamond_proxy_auto…

93d76cc

…mation_registry

udityadav-supraoracles added 2 commits February 20, 2026 11:53

updated unlockDepositAndCycleFee

7a844dd

updated Diamond to not receive native tokens

35ae8f6

sjoshisupra requested changes Feb 20, 2026

View reviewed changes

udityadav-supraoracles added 2 commits February 24, 2026 14:13

resolved PR comments

48a1683

fixed cancel task and stop task

b1f4511

udityadav-supraoracles requested a review from sjoshisupra February 24, 2026 10:29

udityadav-supraoracles added 2 commits February 24, 2026 16:35

updated run.sh for cance task

d420c76

updated AppStorage: using mapping in place of inner structs

6f92967

sjoshisupra requested changes Feb 27, 2026

View reviewed changes

udityadav-supraoracles added 2 commits March 13, 2026 17:22

-Separated conversion logic to ERC20SupraHandler from ERC20Supra

3806953

-Resolved PR comments

- renamed getUserTasks => getTasksByAddress

3d3e1a2

- removed setErc20Supra, setVmSigner

udityadav-supraoracles requested a review from sjoshisupra March 26, 2026 12:37

aregng reviewed Mar 26, 2026

View reviewed changes

sjoshisupra requested changes Mar 27, 2026

View reviewed changes

		if (s.registryState.gasCommittedForNextCycle > _registryMaxGasCap) { revert UnacceptableRegistryMaxGasCap(); }
		if (s.registryState.sysGasCommittedForNextCycle > _sysRegistryMaxGasCap) { revert UnacceptableSysRegistryMaxGasCap(); }

-        for (uint8 i = 0; i < _exponent; i++) {
-            resultScaled = (resultScaled * baseScaled) / DECIMAL;
-        }
+   while (_exponent > 0) {
+     if (_exponent & 0x1) {
+        resultScaled = (resultScaled * baseScaled) / DECIMAL;
+     }
+     else {
+        baseScaled = baseScaled << 1;
+     }
+     _exponent = _exponent >> 1;
+   }

		(bool sent, ) = payable(msg.sender).call{value: _amount}("");
		if (!sent) revert TransferFailed();

Conversation

udityadav-supraoracles commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

udityadav-supraoracles commented Feb 19, 2026

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!

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!

udityadav-supraoracles Feb 23, 2026 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjoshisupra commented Feb 24, 2026

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!

Choose a reason for hiding this comment

Uh oh!

udityadav-supraoracles Mar 10, 2026 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

udityadav-supraoracles commented Feb 17, 2026 •

edited

Loading

udityadav-supraoracles Feb 23, 2026 •

edited

Loading

udityadav-supraoracles Mar 10, 2026 •

edited

Loading