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

fix: correct the use of inputIndex in tests #85

Merged
merged 1 commit into from
Sep 21, 2023
Merged

fix: correct the use of inputIndex in tests #85

merged 1 commit into from
Sep 21, 2023

Conversation

ZzzzHui
Copy link
Contributor

@ZzzzHui ZzzzHui commented Sep 19, 2023 •
edited
Loading

  • correct the naming of inputIndex to inputIndexWithinEpoch if it's used incorrectly
  • abbreviate inputIndexWithinEpoch as IIWE if it's compounded with other words

@ZzzzHui ZzzzHui self-assigned this Sep 19, 2023
@ZzzzHui ZzzzHui linked an issue Sep 19, 2023 that may be closed by this pull request
CartesiDApp.t.sol: Correctly use inputIndex #80
Closed
@ZzzzHui ZzzzHui added this to the 1.1.0 milestone Sep 19, 2023
guidanoli
guidanoli reviewed Sep 19, 2023
View reviewed changes
onchain/rollups/test/foundry/dapp/CartesiDApp.t.sol Outdated
Comment on lines 48 to 49
// IIWE stands for InputIndexWithinEpoch
error IIWEOutOfBounds(uint256 length, uint256 inputIndexWithinEpoch);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a developer (not too familiar with the code) runs forge test and gets this error, they will have no idea what IIWE means. I would, therefore, not abbreviate this error name.

Suggested change
// IIWE stands for InputIndexWithinEpoch
error IIWEOutOfBounds(uint256 length, uint256 inputIndexWithinEpoch);
error InputIndexWithinEpochOutOfBounds(uint256 length, uint256 inputIndexWithinEpoch);

onchain/rollups/test/foundry/dapp/CartesiDApp.t.sol Outdated
Comment on lines 626 to 627
// IIWE stands for InputIndexWithinEpoch
function checkIIWE(uint256 inputIndexWithinEpoch) internal view {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might save some characters, but we lose clarity by abbreviating InputIndexWithinEpoch to IIWE.
I would, therefore, avoid abbreviating it.
This applies to IIWEStr as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. It's always a tradeoff between being clear and concise. Since they don't appear often, we could use the lengthy form

onchain/rollups/test/foundry/util/LibServerManager.sol Outdated
@@ -33,7 +33,7 @@ library LibServerManager {

struct RawProof {
bytes32 context;
string inputIndex;
string inputIndexWithinEpoch;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you take a peek in finish_epoch_response.json, you'll see this is indeed inputIndex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we should modify the proves function in the LibServerManager.sol. Instead of checking Proof.inputIndex, we will be checking Proof.validity.inputIndexWithinEpoch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this makes sense.

@ZzzzHui
Copy link
Contributor Author

ZzzzHui commented Sep 20, 2023 •
edited
Loading

BTW, is there a difference between outputIndex and outputIndexWithinInput? Is outputIndex global like inputIndex is?

@guidanoli
Copy link
Collaborator

BTW, is there a difference between outputIndex and outputIndexWithinInput? Is outputIndex global like inputIndex is?

outputIndexWithinInput lives inside the validity structure, which could change. Meanwhile, inputIndex and outputIndex are in the same level as validity and aim to help filter the correct proof.

@ZzzzHui
Copy link
Contributor Author

ZzzzHui commented Sep 20, 2023

outputIndexWithinInput lives inside the validity structure, which could change

I don't get this part. What change do you mean :)

Apart from being in different levels, is there a difference between outputIndex and outputIndexWithinInput? In other words, should they be 2 different names

@ZzzzHui ZzzzHui force-pushed the feature/correct-input-index branch from 8e70ec5 to 8d7cf23 Compare September 20, 2023 12:05
@ZzzzHui ZzzzHui force-pushed the feature/correct-input-index branch from 8d7cf23 to 276ee8c Compare September 20, 2023 12:09
@guidanoli
Copy link
Collaborator

Apart from being in different levels, is there a difference between outputIndex and outputIndexWithinInput?

I believe they contain the same data.
@gligneul Can you confirm?

@gligneul
Copy link
Contributor

They contain the same data.

@ZzzzHui ZzzzHui merged commit c5db0af into main Sep 21, 2023
4 checks passed
@ZzzzHui ZzzzHui deleted the feature/correct-input-index branch September 21, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guidanoli guidanoli guidanoli approved these changes

@pedroargento pedroargento Awaiting requested review from pedroargento

@xdaniortega xdaniortega Awaiting requested review from xdaniortega

Assignees

@ZzzzHui ZzzzHui

Labels
None yet
Projects
Rollups Unit
Status: 🚀 Done
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

CartesiDApp.t.sol: Correctly use inputIndex
3 participants
@ZzzzHui @guidanoli @gligneul