-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
On-chain node configuration #337
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@cartesi/rollups": minor | ||
--- | ||
|
||
Add data availability configuration to application contract |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// (c) Cartesi and individual authors (see AUTHORS) | ||
// SPDX-License-Identifier: Apache-2.0 (see LICENSE) | ||
|
||
pragma solidity ^0.8.8; | ||
|
||
import {IInputBox} from "../inputs/IInputBox.sol"; | ||
|
||
/// @title Data Availability | ||
/// @notice Defines the signatures of data availability solutions. | ||
interface DataAvailability { | ||
/// @notice The application receives inputs only from | ||
/// a contract that implements the `IInputBox` interface. | ||
/// @param inputBox The input box contract address | ||
function InputBox(IInputBox inputBox) external; | ||
|
||
/// @notice The application receives inputs from | ||
/// a contract that implements the `IInputBox` interface, | ||
/// and from Espresso, starting from a given block height, | ||
/// and for a given namespace ID. | ||
/// @param inputBox The input box contract address | ||
/// @param fromBlock Height of first Espresso block to consider | ||
/// @param namespaceId The Espresso namespace ID | ||
function InputBoxAndEspresso( | ||
IInputBox inputBox, | ||
uint256 fromBlock, | ||
uint32 namespaceId | ||
) external; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,10 @@ contract Application is | |
/// @dev See the `getConsensus` and `migrateToConsensus` functions. | ||
IConsensus internal _consensus; | ||
|
||
/// @notice The data availability solution. | ||
/// @dev See the `getDataAvailability` function. | ||
bytes internal _dataAvailability; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why bytes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, why not a ERC-165 contract? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because with |
||
|
||
/// @notice Creates an `Application` contract. | ||
/// @param consensus The initial consensus contract | ||
/// @param initialOwner The initial application owner | ||
|
@@ -49,10 +53,12 @@ contract Application is | |
constructor( | ||
IConsensus consensus, | ||
address initialOwner, | ||
bytes32 templateHash | ||
bytes32 templateHash, | ||
bytes memory dataAvailability | ||
) Ownable(initialOwner) { | ||
_templateHash = templateHash; | ||
_consensus = consensus; | ||
_dataAvailability = dataAvailability; | ||
} | ||
|
||
/// @notice Accept Ether transfers. | ||
|
@@ -136,6 +142,15 @@ contract Application is | |
return _consensus; | ||
} | ||
|
||
function getDataAvailability() | ||
external | ||
view | ||
override | ||
returns (bytes memory) | ||
{ | ||
return _dataAvailability; | ||
} | ||
|
||
function owner() public view override(IOwnable, Ownable) returns (address) { | ||
return super.owner(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure
InputBoxAndEspresso
should be part of a global officialDataAvailability
interface. I am expecting people to be able to add their own without messing with our contracts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to have at least the ones we fully support defined here so that deployment front-ends and the node can utilize the same definitions. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can still provide their own DA config blobs on deployment, and hack their own DA provider node plug-in that understands and decodes that blob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if someone wants to add a new DA signature, they can just open a PR. It won't affect the bytecode of our deployed contracts, and therefore won't trigger any redeployments. Therefore, we can release minor versions with these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see. But "fully supporting" an integration is a complicated term, and I'm still not very comfortable with our regular contracts mentioning specific integrations/extensions, I feel it should be separate. Couldn't these signatures be placed in separate interfaces?
I'd like to hear other opinions, maybe @pedroargento or @tuler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could totally place these definitions in a third place, but it is convenient to place them here because the
rollups-node
repo already uses the artifacts fromrollups-contracts
(which includes contracts/interface ABIs), and front-ends often depend on the@cartesi/rollups
npm package to interact with our contracts (e.g. when deploying).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah, we could create a new repo (e.g.
rollups-da-config
) with just this file, which would not be imported byrollups-contracts
, but would be imported/used by the node and deployment front-ends.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our regular contracts are not mentioning integrations/extensions. If you were to draw a dependency graph of the
rollups-contracts
with this PR, this interface would be isolated from every other contract. It would not be referenced by any other contract, and therefore not influence any of them. That's why we could, in theory, move it to a separate repo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear that I'm not advocating to moving it to somewhere else. I think it's okay to keep it here. If this interface grows out of control, we can think about create a dedicated repo for it.