Inheritence Refactoring #442

guidanoli · 2025-09-23T19:26:50Z

No description provided.

socket-security · 2025-09-23T19:27:25Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@usecannon/cli@2.22.0 ⏵ 2.24.2	^-3		⁺¹	^-6
	@changesets/cli@2.29.4 ⏵ 2.29.5				⁺²

View full report

GCdePaula · 2025-09-26T16:28:16Z

Hey @guidanoli! Great work on this PR :)

I've a few requests and comments!

Do you have some schematics/diagrams of this inheritance design? I'm having difficulties seeing who inherits who, and where the functions are being implemented. I think with such a diagram, we'd be able to have a high-level view of the architecture as a whole.

Take the _isOutputsRootFinal virtual method defined on the OutboxImpl. It is overridden/implemented on the DaveAppImpl, which then calls the EpochManagerImpl's method isOutputsRootFinal, which is defined on the EpochManagerImpl interface. This is getting hard to track, and I fear may be getting into the "excess of abstraction" zone.

I see a pattern where view functions called canDoAction don't return a bool, but reverts if the action cannot be performed. I think this pattern may cause issues on the node, specially on the finalize/close view methods.

Can you describe, perhaps also with the aid of a schematic, the design of epochs closing and finalizing?

guidanoli · 2025-09-30T21:00:24Z

Thanks for the valuable feedback, @GCdePaula !

I have simplified the inheritance graph, I hope you can see it clearly in the source code.
To ease visualization, I can later plot the graph using Mermaid here.

Could you expand on why the pattern of view methods that revert if an action cannot be performed might cause troubles with the node? This strategy aimed to simplify the implementation and readability of the contracts. On eth_call requests, the node can check whether the call reverted and why (data), which it could then decode into the appropriate Solidity error. All this should be done automatically by the bindings library.

GCdePaula · 2025-09-30T22:04:42Z

Ok! Thank you. I'm taking a look :)

Is the epoch close+finalize design described anywhere?

pedroargento · 2025-10-01T02:42:59Z

I am not sure I like the name EventEmitter. Its not at all related to what the contract does, which is store the deployment block number. The name is related to the offchain client but it doesn't make a lot of sense in this codebase. Also, every smart contract is a event emitter.

Maybe something like DeploymentBlock or DeploymentData?

guidanoli · 2025-10-01T10:18:14Z

Every contract can emit events, but not every contract does.
For those contracts that do emit events, we provide their deployment block number.
The only reason we provide this info is for off-chain nodes to have a better lower bound than the genesis block.

In the code base, it's easier to understand why Inbox inherits from EventEmitter: It emits events. By reading the documentation on EventEmitter, the curious reader can understand the purpose of this interface.

guidanoli · 2025-10-01T10:27:07Z

Is the epoch close+finalize design described anywhere?

It is thoroughly described in the documentation of the EpochManager interface.

guidanoli · 2025-10-01T10:56:17Z

🌳 Inheritance Tree

Here is the inheritance tree in Mermaid.
Note how it neatly follows the Rollups pipeline:

Inputs are received by InboxImpl
Inputs are divided into epochs by EpochManagerImpl
Epochs are finalized (somehow), allowing output validation/execution by OutboxImpl
Epoch finalization logic is defined by DaveAppImpl and QuorumAppImpl

graph TD
    OutboxImpl --> EpochManagerImpl & ReentrancyGuard:::oz
    EpochManagerImpl --> InboxImpl
    InboxImpl --> DeploymentInfoProviderImpl
    TokenReceiverImpl --> ERC721Holder:::oz & ERC1155Holder:::oz
    AppImpl["{Dave,Quorum}AppImpl"] --> OutboxImpl & TokenReceiverImpl
    classDef oz fill:cyan,color:black

PS: OpenZeppelin contracts are colored cyan.

🧠 Rationale

The leaf node is DeploymentInfoProviderImpl, because it doesn't depend on any other contract.
InboxImpl inherits from DeploymentInfoProviderImpl because it provides deployment info.
EpochManagerImpl inherits from InboxImpl because it internally stores epoch boundaries in terms of input indices (used by PRT, and to ensure closed epochs are non-empty).
OutboxImpl inherits from EpochManagerImpl because it needs to check whether any given post-epoch outputs root has been finalized before (when validating an output). It also inherits from ReentrancyGuard to avoid reentrancy attacks during output execution.
DaveAppImpl inherits from OutboxImpl (which inherits from EpochManagerImpl and InboxImpl) because it needs to have access to input Merkle roots and epoch boundaries, in order to implement the IDataProvider interface.
Both app implementations also inherit from TokenReceiverImpl in order to receive Ether, ERC-721, and ERC-1155 tokens (note that the ERC-20 standard doesn't require a contract to implement any "holder" logic).

🗒️ Notes

EpochManagerImpl declares an internal virtual _isPostEpochStateRootValid, which is implemented differently by QuorumAppImpl and DaveAppImpl, because it heavily depends on the consensus algorithm used by the application.
EpochManagerImpl also defines several internal functions that are used by both app implementations, such as _closeEpoch, _preFinalize, _finalizeEpoch, _getInputIndex{InclusiveLower,ExclusiveUpper}Bound, and _isFirstNonFinalizedEpochClosed.

guidanoli · 2025-10-06T15:45:37Z

I've renamed getNumberOf<X> view functions as get<X>Count and added:

getClosedEpochCount view function, to monitor EpochClosed events
getDeployedAppCount view function, to monitor {Dave,Quorum}AppDeployed events

src/portals/IPortal.sol

tuler · 2025-10-07T18:56:01Z

src/app/interfaces/Inbox.sol

+    function getInputCount() external view returns (uint256);
+
+    /// @notice Get the number of inputs before the current block.
+    function getInputCountBeforeCurrentBlock() external view returns (uint256);


Why "BeforeCurrentBlock"? Isn't that implicit?

A transaction in the same block may alter the count. This function will ignore it if it happens.

This function is used internally to determine epoch boundaries in terms of inputs.

tuler · 2025-10-07T18:58:12Z

src/app/interfaces/EpochManager.sol

+interface EpochManager is EventEmitter {
+    /// @notice An epoch has been closed.
+    /// @param epochIndex The index of the epoch
+    /// @param epochFinalizer The contract that makes the epoch reach finality


if it's a contract why is it an address and not a contract interface?

Because Solidity doesn't support generics, like EpochManager<T>, where T is the type of the epoch finalizer contract. The interface ID of the epoch finalizer contract is provided by the getEpochFinalizerInterfaceId view function.

tuler · 2025-10-07T19:20:09Z

src/app/concretes/QuorumAppImpl.sol

+        // If not, mark the validator as having already voted in the epoch.
+        {
+            bytes32 bitmap = getAggregatedVoteBitmap(epochIndex);
+            require(!bitmap.getBitAt(validatorId), VoteAlreadyCastForEpoch());


is this error necessary?

Yes, this error actually ensures that a validator cannot vote on multiple post-epoch state roots for any given epoch.

ZzzzHui · 2025-10-08T04:31:48Z

I am not sure I like the name EventEmitter. Its not at all related to what the contract does, which is store the deployment block number. The name is related to the offchain client but it doesn't make a lot of sense in this codebase. Also, every smart contract is a event emitter.

Maybe something like DeploymentBlock or DeploymentData?

I also think EventEmitter is quite general. We could consider using what Pedro suggested and I would add DeploymentInfo on top of his list

guidanoli · 2025-10-08T13:16:29Z

I also think EventEmitter is quite general. We could consider using what Pedro suggested and I would add DeploymentInfo on top of his list.

Okay, how about DeploymentInfoProvider?

ZzzzHui · 2025-10-09T04:59:00Z

src/app/abstracts/DeploymentInfoProviderImpl.sol

+        public
+        view
+        override
+        returns (uint256 deploymentBlockNumber)


probably no need for variable deploymentBlockNumber

ZzzzHui · 2025-10-09T05:06:03Z

src/app/abstracts/EpochManagerImpl.sol

+    using LibBinaryMerkleTree for bytes32[];
+
+    uint256 private _finalizedEpochCount;
+    bool private __isFirstNonFinalizedEpochClosed;


this variable has one extra _

ZzzzHui · 2025-10-09T05:12:17Z

src/app/abstracts/EpochManagerImpl.sol

+        public
+        view
+        override
+        returns (uint256 closedEpochCount)


probably no need for variable closedEpochCount, same for its interface function

ZzzzHui · 2025-10-09T05:18:02Z

src/app/abstracts/EpochManagerImpl.sol

+        returns (uint256 inputIndexInclusiveLowerBound)
+    {
+        return _inputIndexInclusiveLowerBound;
+    }
+
+    /// @notice Get the input index exclusive upper bound.
+    function _getInputIndexExclusiveUpperBound()
+        internal
+        view
+        returns (uint256 inputIndexExclusiveUpperBound)
+    {
+        return _inputIndexExclusiveUpperBound;
+    }
+
+    /// @notice Check whether the first non-finalized epoch is closed.
+    function _isFirstNonFinalizedEpochClosed()
+        internal
+        view
+        returns (bool isFirstNonFinalizedEpochClosed)


same for these variables

ZzzzHui · 2025-10-09T05:33:32Z

src/app/abstracts/EpochManagerImpl.sol

+        internal
+    {
+        uint256 epochIndex = _finalizedEpochCount;
+        _finalizedEpochCount = epochIndex + 1;


it may be easier to read if we write it as ++_finalizedEpochCount;

ZzzzHui · 2025-10-09T06:41:11Z

src/app/concretes/DaveAppFactoryImpl.sol

+        returns (uint256 deployedAppCount)
+    {
+        return _deployedAppCount;
+    }
+
+    function computeDaveAppAddress(bytes32 genesisStateRoot, bytes32 salt)
+        external
+        view
+        override
+        returns (address appAddress)


guidanoli added 17 commits September 23, 2025 15:51

Bump Cannonfile

f554c2b

Remove tests and bump forge-std

ccd7706

Bump NPM dependencies

b91f4ae

Define interface for SafeERC20Transfer

42c913d

Reuse constants from machine-solidity-step

9b59188

Rename canonical machine constants

bad4ea1

Test LibAddress

3f8b25d

Remove old contracts from Cannon build

e63b083

Test InputEncoding

1ae6095

Add and test LibBitmap

bce1e8b

Test with Cannon

46c5a09

Add new app contracts

89c2315

Modify portals

0d49c7a

Add LibCannon

dd5ffa2

Import PRT contracts for Cannon build

3121d75

Test new app contracts

08c062c

Remove old contracts

c989d23

guidanoli requested a review from pedroargento September 23, 2025 19:26

guidanoli self-assigned this Sep 23, 2025

guidanoli added 4 commits September 26, 2025 18:10

Make InboxImpl inherit from EventEmitterImpl

d37a2c4

Make EpochManagerImpl inherit from InboxImpl

2022d85

Remove virtual functions

d750fcf

Remove DaveImpl and QuorumImpl

a633592

guidanoli added 3 commits October 6, 2025 10:38

Rewrite "number of inputs" as "input count"

bbd0f81

Rewrite "number of validators" as "validator count"

8e5365b

Rewrite "number of executed outputs" as "output execution count"

495007b

guidanoli requested review from ZzzzHui and tuler October 6, 2025 14:01

guidanoli added 3 commits October 6, 2025 11:03

Rewrite "num(ber) of X" as "X count"

63df112

Add getClosedEpochCount view function

36b1fe2

Add getDeployedAppCount view function to app factories

426a7b3

guidanoli mentioned this pull request Oct 7, 2025

rename DaveConsensus to OneOfN #436

Closed

tuler reviewed Oct 7, 2025

View reviewed changes

guidanoli added 2 commits October 8, 2025 10:32

EventEmitter -> DeploymentInfoProvider

e794b21

Remove Portal and IPortal

8132c08

guidanoli requested a review from tuler October 8, 2025 13:41

ZzzzHui reviewed Oct 9, 2025

View reviewed changes

This was referenced Oct 14, 2025

Design unified consensus interface #426

Open

Make outputs Merkle root validator immutable #414

Open

Inheritence Refactoring #442

Are you sure you want to change the base?

Inheritence Refactoring #442

Conversation

guidanoli commented Sep 23, 2025

Uh oh!

socket-security bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GCdePaula commented Sep 26, 2025

Uh oh!

guidanoli commented Sep 30, 2025

Uh oh!

GCdePaula commented Sep 30, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

guidanoli commented Oct 1, 2025

Uh oh!

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

Uh oh!

🌳 Inheritance Tree

🧠 Rationale

🗒️ Notes

Uh oh!

guidanoli commented Oct 6, 2025

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guidanoli Oct 8, 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!

ZzzzHui commented Oct 8, 2025

Uh oh!

guidanoli commented Oct 8, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

socket-security bot commented Sep 23, 2025 •

edited

Loading

pedroargento commented Oct 1, 2025 •

edited

Loading

guidanoli commented Oct 1, 2025 •

edited

Loading

guidanoli commented Oct 1, 2025 •

edited

Loading

guidanoli Oct 8, 2025 •

edited

Loading