Skip to content
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

Merged
merged 2 commits into from
Jan 14, 2025
Merged

On-chain node configuration #337

merged 2 commits into from
Jan 14, 2025

Conversation

guidanoli
Copy link
Collaborator

No description provided.

@guidanoli guidanoli self-assigned this Dec 13, 2024
@guidanoli guidanoli force-pushed the feature/onchain-node-config branch from 2a14fec to 7cb08d8 Compare December 13, 2024 23:58
/// @param inputBox The input box contract address
/// @param fromBlock Height of first Espresso block to consider
/// @param namespaceId The Espresso namespace ID
function InputBoxAndEspresso(
Copy link
Contributor

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 official DataAvailability interface. I am expecting people to be able to add their own without messing with our contracts.

Copy link
Collaborator Author

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. :-)

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@guidanoli guidanoli Dec 16, 2024

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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 from rollups-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).

Copy link
Collaborator Author

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 by rollups-contracts, but would be imported/used by the node and deployment front-ends.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not very comfortable with our regular contracts mentioning specific integrations/extensions

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.

Copy link
Collaborator Author

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.

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why bytes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, why not a ERC-165 contract?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because with bytes we can encode more data specific to that application.
For example, applications that use Espresso would specify their namespace and Espresso block height.

@tuler
Copy link
Member

tuler commented Dec 17, 2024

I'm not a solidity dev to judge this well.
But it feels very weird this generic bytes field, when what you have is polymorphism.

In a sane language, which I guess it not solidity, I'd expect a usage like:

IDataAvailability da = app.getDataAvailability();
if (da instanceof IEspressoDataAvailability) {
  IEspressoDataAvailability eda = (IEspressoDataAvailability) da;
  eda.getFromBlock();
  eda.getNamespaceId();
}

@guidanoli
Copy link
Collaborator Author

Yes, this solution could work, but then you would have to deploy this data availability contract for every application that uses Espresso. In reality, we don't need polymorphism. We only need algebraic data types, as in Haskell or Rust:

enum DataAvailabilityConfig {
  InputBox { inputBoxContract: Address },
  InputBoxAndEspresso { inputBoxContract: Address, fromBlock: Uint256, namespaceId: Uint32 },
}

The way we are encoding algebraic data types in Solidity is as ABI-encoded function calls.

@tuler
Copy link
Member

tuler commented Dec 17, 2024

The way we are encoding algebraic data types in Solidity is as ABI-encoded function calls.

You have to agree that is a very non-idiomatic way of expressing algebraic types. 😵‍💫

@guidanoli
Copy link
Collaborator Author

You have to agree that is a very non-idiomatic way of expressing algebraic types. 😵‍💫

Unfortunately, that is the best way I know of expressing algebraic types in Solidity.

@miltonjonat
Copy link
Contributor

@guidanoli @pedroargento so what's the conclusion here? Are we doing something with ERC-165 in the end, or sticking with bytes? We need to move on this one really soon, since the initial Node v2 release will use this.
@vfusco FYI

@guidanoli
Copy link
Collaborator Author

Hey Milton, you're right.
We should decide which strategy are we using.
I'm personally leaning towards ERC-165, because of its elegance and cost.
Let's wait and see what @pedroargento, @ZzzzHui and @vfusco think as well.

@ZzzzHui
Copy link
Contributor

ZzzzHui commented Jan 9, 2025

I side with Pedro that this solution is more straightforward than the ERC165 one

@miltonjonat
Copy link
Contributor

I've talked to @vfusco and he is a bit concerned about the impact of the ERC-165 solution on the current Node's code. It is mandatory for us to release a Node version in the coming weeks, and it would be really important to include some on-chain config solution.
So on our side, we are leaning towards this solution as well, at least for the time being. We could consider the ERC-165 one at a later time, when contracts are again updated due to Dave etc.

@guidanoli
Copy link
Collaborator Author

Alright, having seen that the majority prefers the ABI-encoded function call, let's move with this solution.

@guidanoli guidanoli merged commit ecc77a9 into main Jan 14, 2025
3 checks passed
@guidanoli guidanoli deleted the feature/onchain-node-config branch January 14, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants