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

setup for day1 #13

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

setup for day1 #13

wants to merge 15 commits into from

Conversation

nd-certora
Copy link

Description

Notice

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you assigned this PR to yourself?
  • Have you added at least 1 reviewer?
  • Have you updated the official documentation?
  • Have you added sufficient documentation in your code?
  • Have you added relevant tests to the official test suite?

Pull Request Type

  • 💫 New Feature (Breaking Change)
  • 💫 New Feature (Non-breaking Change)
  • 🛠️ Bug fix (Non-breaking Change: Fixes an issue)
  • 🕹️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)

Breaking changes (if applicable)

Testing

  • Have you tested this code with the official test suite?
  • Have you tested this code manually?

Manual tests (if applicable)

Additional comments

Copy link
Collaborator

@otakar-trunda otakar-trunda left a comment

Choose a reason for hiding this comment

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

Looks good. I left some minor comments.

I already started to work on the examples as well. Here's my code https://github.com/Certora/liquid-collective-protocol/blob/otakar/training/certora/specs/OperatorRegistryV1.spec
I like your rules better but maybe this can still help. There's also a parametric rule for day 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of the harness methods are not needed for the training.

Copy link
Author

Choose a reason for hiding this comment

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

yes, but I think they are not confusing. I did change the returned type of getOperatorState , do you think it is good? maybe need to adopt your spec as well?

Comment on lines 41 to 42
uint32 limitAfter;
_, limitAfter, _, _, _, _, _ = getOperatorState(e, opIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an easier way to write this.
uint32 limitAfter = getOperator(opIndex).limit;


uint32 limitAfter;
_, limitAfter, _, _, _, _, _ = getOperatorState(opIndex);
satisfy limitAfter == limitBefore;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be satisfied by choosing newLimits[i] = limitBefore, right? Is that what we want to show?

A witness for changing the variable would make more sense to me. I.e. satisfy limitAfter != limitBefore to show that rule integritySetOperatorLimits is not vacuous.

Copy link
Author

Choose a reason for hiding this comment

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

exactly, I want us to try many cases, and also notice when we want the separate satisfy

uint32 limitBefore;
uint256 latestKeysEditBlockNumber;

_, limitBefore, _, _, _, _, _ = getOperatorState(opIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be simplified via getOperator(opIndex).limit

Copy link
Author

Choose a reason for hiding this comment

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

nice, it's working

uint256 latestKeysEditBlockNumber;

_, limitBefore, _, _, _, _, _ = getOperatorState(opIndex);
latestKeysEditBlockNumber = getLatestKeysEditBlockNumber(opIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can also be simplified as getOperator(opIndex).latestKeysEditBlockNumber - without the need for a harness method, but this looks fine too.
Also the variable latestKeysEditBlockNumber is never used but I guess you plan to use it during the lesson somehow.

Copy link
Author

Choose a reason for hiding this comment

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

nice


import "../../contracts/src/OperatorsRegistry.1.sol";
import "../../contracts/src/state/operatorsRegistry/Operators.2.sol";
contract OperatorsRegistryV1Harness is OperatorsRegistryV1 {

Choose a reason for hiding this comment

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

I think we should only keep the methods we need in the harness.


/**
@title - integrity of a successful (non reverting) to setOperatorLimits()
// todo - violated, undersntad why and fix

Choose a reason for hiding this comment

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

undersntad -> understand

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.

3 participants