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

Fix: chi-01 (agent root passing) #1526

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Conversation

ChiTimesChi
Copy link
Collaborator

@ChiTimesChi ChiTimesChi commented Oct 31, 2023

Description
Replaces #1331. The merge conflicts there were a bit annoying to solve, so given the low diff size decided to redo the PR.

List of old commits and their new counterparts

Summary by CodeRabbit

Refactor:

  • Simplified the passAgentRoot function in the Destination contract and its interface InterfaceDestination. It now only returns a boolean indicating if there is a pending agent root, enhancing the function's clarity and focus.

Tests:

  • Updated the DestinationTest and DestinationSynapseTest contracts to reflect the changes in the passAgentRoot function. This ensures our tests remain accurate and reliable.
  • Enhanced the test_acceptAttestation_passesAgentRoot function in the DestinationTest contract to include logic for forming and submitting a second attestation, improving the test's comprehensiveness.

@ChiTimesChi ChiTimesChi requested a review from trajan0x as a code owner October 31, 2023 22:47
Copy link
Contributor

coderabbitai bot commented Oct 31, 2023

Walkthrough

The primary change revolves around the passAgentRoot function across multiple contracts and interfaces. The function's return value has been simplified to only indicate if there is a pending agent root. This modification impacts the way attestations are accepted and tested, with adjustments made to relevant test suites.

Changes

File Path Summary
.../contracts/Destination.sol Removed the check for passing the current agent merkle root before accepting an attestation. Modified the passAgentRoot() function to only return a boolean indicating if there is a pending root.
.../contracts/interfaces/InterfaceDestination.sol Altered the passAgentRoot function to only return rootPending, removing rootPassed.
.../test/mocks/DestinationMock.t.sol Updated the passAgentRoot function to return only rootPending.
.../test/suite/Destination.t.sol Multiple changes to the DestinationTest contract, primarily around the passAgentRoot function and the test_acceptAttestation_passesAgentRoot function.
.../test/suite/DestinationSynapse.t.sol Removed bool rootPassed from the DestinationSynapseTest contract. The dst.passAgentRoot() function now directly assigns its return value to bool rootPending.

🐇🍂

In the season of fall, when the leaves gently sway,
CodeRabbit hops along, making changes on the way.

Simplifying functions, making code neat,
In the world of contracts, that's quite a feat!

Celebrate the changes, for they bring clarity,
To the complex world of solidity, a moment of rarity.

So here's to the code, ever evolving and new,
Happy Halloween to all, and to all a hearty boo! 🎃👻

Validation with GitHub issue(Beta)

Assessment of the PR changes against GitHub Issue

Aspect Aligns with the linked issue Explanation
passAgentRoot function refactoring ☑️ The function has been refactored to return a single boolean value instead of a tuple, aligning with the issue's requirement.
Handling of attestation and agent root ☑️ The code changes indicate that the attestation is now handled before passing the agent root, which aligns with the issue's requirement.
Test updates ☑️ The tests have been updated and new ones have been added to ensure the correct behavior of the updated function and the handling of different scenarios, as required by the issue.
Metadata updates The pull request does not mention any updates to the metadata of the contracts, which is a requirement mentioned in the issue.
Checks for changes in agent root, dispute status, and end of optimistic period The pull request does not provide clear information about these checks, so it's uncertain if these changes align with the issue's requirements.
 </details><!-- This is an auto-generated comment: raw summary by coderabbit.ai -->

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

@github-actions github-actions bot added M-contracts Module: Contracts 2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. size/s labels Oct 31, 2023
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d175290) 50.81212% compared to head (ba27000) 50.81006%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #1526         +/-   ##
===================================================
- Coverage   50.81212%   50.81006%   -0.00207%     
===================================================
  Files            357         357                 
  Lines          23888       23887          -1     
  Branches         280         279          -1     
===================================================
- Hits           12138       12137          -1     
  Misses         10596       10596                 
  Partials        1154        1154                 
Flag Coverage Δ
packages 90.62003% <ø> (ø)
solidity 93.63708% <100.00000%> (-0.00500%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/contracts-core/contracts/Destination.sol 93.75000% <100.00000%> (-0.09615%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between cc87960 and ba27000.
Files selected for processing (5)
  • packages/contracts-core/contracts/Destination.sol (3 hunks)
  • packages/contracts-core/contracts/interfaces/InterfaceDestination.sol (1 hunks)
  • packages/contracts-core/test/mocks/DestinationMock.t.sol (1 hunks)
  • packages/contracts-core/test/suite/Destination.t.sol (10 hunks)
  • packages/contracts-core/test/suite/DestinationSynapse.t.sol (1 hunks)
Additional comments: 16
packages/contracts-core/contracts/interfaces/InterfaceDestination.sol (1)
  • 7-12: The function signature and documentation have been updated to reflect the new behavior. Ensure that all calls to this function throughout the codebase have been updated to handle the new return value.
packages/contracts-core/test/mocks/DestinationMock.t.sol (1)
  • 13-13: The function signature has been updated to reflect the changes in the return values. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- function passAgentRoot() external returns (bool rootPassed, bool rootPending) {}
+ function passAgentRoot() external returns (bool rootPending) {}
packages/contracts-core/test/suite/DestinationSynapse.t.sol (1)
  • 33-38: The test test_setupCorrectly has been updated to reflect the changes in the passAgentRoot function. The function now returns a boolean indicating whether there is a pending agent root. The assertion assertFalse(rootPending) checks that there is no pending agent root at the time of setup.
packages/contracts-core/contracts/Destination.sol (3)
  • 98-103: The check for passing the current agent merkle root before accepting an attestation has been removed. This change simplifies the logic and reduces gas costs, but it also means that the function will now accept attestations even if the current agent merkle root has not been passed. Ensure that this change does not introduce any security vulnerabilities or inconsistencies in the contract's state.

  • 108-114: The return values of the passAgentRoot() function have been modified to only return a boolean indicating whether there is a pending root or not. This change simplifies the function's interface and makes it easier to use, but it also removes the ability to check whether the current agent merkle root was passed to the LightManager. Ensure that this change does not break any code that relies on the old return values.

- _saveAttestation(att, notaryIndex, sigIndex);
- _storedAttestations.push(StoredAttData({agentRoot: agentRoot, dataHash: att.dataHash()}));
- // Save Agent Root if required, and update the Destination's Status
- bool rootPending = passAgentRoot();
- destStatus = _saveAgentRoot(rootPending, agentRoot, notaryIndex);
- _saveGasData(snapGas, notaryIndex);
- return true;
+ _saveAttestation(att, notaryIndex, sigIndex);
+ _storedAttestations.push(StoredAttData({agentRoot: agentRoot, dataHash: att.dataHash()}));
+ // Save Agent Root if required, and update the Destination's Status
+ bool rootPending = passAgentRoot();
+ destStatus = _saveAgentRoot(rootPending, agentRoot, notaryIndex);
+ _saveGasData(snapGas, notaryIndex);
+ return true;
  • 117-150: The logic in the passAgentRoot() function has been updated to reflect the changes in its return values. The function now only checks whether there is a pending agent root, and does not check whether the current agent merkle root was passed to the LightManager. This change simplifies the function's logic and reduces gas costs, but it also removes the ability to check whether the current agent merkle root was passed to the LightManager. Ensure that this change does not introduce any security vulnerabilities or inconsistencies in the contract's state.
- function passAgentRoot() public returns (bool rootPending) {
-     // Agent root is not passed on Synapse Chain, as it could be accessed via BondingManager
-     if (localDomain == synapseDomain) return false;
-     bytes32 oldRoot = IAgentManager(agentManager).agentRoot();
-     bytes32 newRoot = _nextAgentRoot;
-     // Check if agent root differs from the current one in LightManager
-     if (oldRoot == newRoot) return false;
-     DestinationStatus memory status = destStatus;
-     // Invariant: Notary who supplied `newRoot` was registered as active against `oldRoot`
-     // So we just need to check the Dispute status of the Notary
-     if (_notaryDisputeExists(status.notaryIndex)) {
-         // Remove the pending agent merkle root, as its signer is in dispute
-         _nextAgentRoot = oldRoot;
-         return false;
-     }
-     // If Notary recently won a Dispute, we can optimistically assume that their passed root is valid.
-     // However, we need to wait until the Dispute timeout is over, before passing the new root to LightManager.
-     if (_notaryDisputeTimeout(status.notaryIndex)) {
-         // We didn't pass anything, but there is a pending root
-         return true;
-     }
-     // Check if agent root optimistic period is over
-     if (status.agentRootTime + AGENT_ROOT_OPTIMISTIC_PERIOD > block.timestamp) {
-         // We didn't pass anything, but there is a pending root
-         return true;
-     }
-     // `newRoot` signer was not disputed, and the root optimistic period is over.
-     // Finally, pass the Agent Merkle Root to LightManager
-     InterfaceLightManager(address(agentManager)).setAgentRoot(newRoot);
-     return false;
- }
+ function passAgentRoot() public returns (bool rootPending) {
+     // Agent root is not passed on Synapse Chain, as it could be accessed via BondingManager
+     if (localDomain == synapseDomain) return false;
+     bytes32 oldRoot = IAgentManager(agentManager).agentRoot();
+     bytes32 newRoot = _nextAgentRoot;
+     // Check if agent root differs from the current one in LightManager
+     if (oldRoot == newRoot) return false;
+     DestinationStatus memory status = destStatus;
+     // Invariant: Notary who supplied `newRoot` was registered as active against `oldRoot`
+     // So we just need to check the Dispute status of the Notary
+     if (_notaryDisputeExists(status.notaryIndex)) {
+         // Remove the pending agent merkle


</blockquote></details>
<details><summary>packages/contracts-core/test/suite/Destination.t.sol (10)</summary><blockquote>

* 44-45: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 264-294: The function `test_acceptAttestation_passesAgentRoot` has been significantly updated. It now includes logic to form and submit a second attestation, checks for updated agent root and index values in the `destStatus` function, and checks the pending agent root in the `nextAgentRoot` function. Ensure that these changes are consistent with the intended behavior.



* 332-338: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 341-347: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 375-382: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 387-394: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 403-410: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 419-426: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 435-442: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



* 450-457: The return value of `passAgentRoot` is now directly assigned to `bool rootPending`. This change is consistent with the PR summary.



</blockquote></details></blockquote></details>



</details>

@ChiTimesChi ChiTimesChi merged commit f7ccb5c into master Oct 31, 2023
51 checks passed
@ChiTimesChi ChiTimesChi deleted the fix/chi/01-agent-root-passing branch October 31, 2023 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. M-contracts Module: Contracts needs-go-generate-agents size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants