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.

Sign up for GitHub

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

Jump to bottom

feat!: output unification #43

Merged
merged 3 commits into from
Jan 23, 2024
Merged

feat!: output unification #43

merged 3 commits into from
Jan 23, 2024

Conversation

guidanoli
Copy link
Collaborator

Closes #42

@guidanoli guidanoli self-assigned this Aug 28, 2023
@guidanoli guidanoli linked an issue Aug 28, 2023 that may be closed by this pull request
Output Unification #42
Closed
@guidanoli guidanoli mentioned this pull request Aug 28, 2023
Closed
@guidanoli guidanoli changed the title refactor!: output unification feat!: output unification Aug 28, 2023
Base automatically changed from feature/adapt-workflows to main August 28, 2023 12:18
guidanoli
guidanoli commented Sep 14, 2023
View reviewed changes
onchain/rollups/contracts/library/LibOutputValidation.sol Outdated
@@ -105,7 +190,7 @@ library LibOutputValidation {
// is contained in it. We can't simply use hashOfOutput because the
// log2size of the leaf is three (8 bytes) not five (32 bytes)
bytes32 merkleRootOfHashOfOutput = MerkleV2.getMerkleRootFromBytes(
abi.encodePacked(keccak256(encodedOutput)),
abi.encodePacked(keccak256(abi.encode(output))),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this is a temporary solution to generate correct output validity proofs. We would like to hash output directly, without having to ABI-encode it.

onchain/rollups/test/foundry/dapp/CartesiDApp.t.sol Outdated
function writeInputs() internal {
for (uint256 i; i < outputEnums.length; ++i) {
LibServerManager.OutputEnum outputEnum = outputEnums[i];
if (outputEnum == LibServerManager.OutputEnum.VOUCHER) {
Voucher memory voucher = getVoucher(i);
writeInput(i, voucher.destination, voucher.payload);
writeInput(i, noticeSender, this.encodeVoucher(voucher));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: This is a temporary solution to generate correct proofs. The new echo DApp should allow us to generate whatever vouchers and notices we need, as well as any output we might come up with in the future.

@guidanoli guidanoli changed the base branch from main to next October 27, 2023 18:47
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 2, 2023

⚠️ No Changeset found

Latest commit: 8c0a23e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ZzzzHui ZzzzHui changed the base branch from next to feature/solidity-style January 12, 2024 16:13
@ZzzzHui ZzzzHui changed the base branch from feature/solidity-style to next January 12, 2024 16:15
ZzzzHui
ZzzzHui reviewed Jan 12, 2024
View reviewed changes
onchain/rollups/contracts/library/LibOutputValidation.sol Outdated
/// xxxxxxxxxxxxxxx │
/// xxxxxxxxxxxxxxxxx
/// ┌────────┬─┬────────┐
/// │ ... │┼│ ... │ ───> For each input in the epoch (G)
Copy link
Contributor

Choose a reason for hiding this comment

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

Before rebase, (G) appears again in the struct OutputValidityProof's comments, i.e.

/// @param inputIndexWithinEpoch Which input, inside the epoch, the output belongs to (G)

But because the struct OutputValidityProof is moved to a new file after rebase, this reference is also removed

@guidanoli guidanoli changed the base branch from next to main January 22, 2024 21:32
@guidanoli guidanoli marked this pull request as ready for review January 22, 2024 21:36
ZzzzHui
ZzzzHui approved these changes Jan 23, 2024
View reviewed changes
onchain/rollups/test/foundry/dapp/Application.t.sol
@@ -978,8 +992,7 @@ contract ApplicationTest is TestBase {
inputIndexWithinEpoch: v.inputIndexWithinEpoch.toUint64(),
outputIndexWithinInput: v.outputIndexWithinInput.toUint64(),
outputHashesRootHash: v.outputHashesRootHash,
vouchersEpochRootHash: v.vouchersEpochRootHash,
noticesEpochRootHash: v.noticesEpochRootHash,
outputsEpochRootHash: v.noticesEpochRootHash,
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the off-chain component is ready, we should update this and encoding and the helper library LibServerManager

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, many lines in this PR are mere artifacts that let us test the new output validation scheme using the old machine.

@guidanoli guidanoli merged commit e26b527 into main Jan 23, 2024
3 checks passed
@guidanoli guidanoli deleted the feature/output-unification branch January 23, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZzzzHui ZzzzHui ZzzzHui approved these changes

@pedroargento pedroargento Awaiting requested review from pedroargento

Assignees

@guidanoli guidanoli

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Output Unification
2 participants
@guidanoli @ZzzzHui