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

Test Coverage #43

Merged
merged 10 commits into from
May 28, 2024
Merged

Test Coverage #43

merged 10 commits into from
May 28, 2024

Conversation

TokenTitan
Copy link
Contributor

No description provided.

TokenTitan and others added 7 commits May 23, 2024 15:32
* feat: completed call breaker deployment script

* feat: added laminator script

* feat: deploy script for smarted contract

* refactor: formating

* feat: added deploy script for SelfCheckout

* refactor

* fix: format

* feat: added crontwoCounter deployment script

* refactor: renaming and removing unused variables

* feat: added base and sepolia to networks

* refactor: removed todo comment
@TokenTitan TokenTitan marked this pull request as ready for review May 23, 2024 15:58
if (owner() != address(0)) {
revert AlreadyInit();
}
if (_owner == address(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing these checks?

Copy link
Contributor Author

@TokenTitan TokenTitan May 28, 2024

Choose a reason for hiding this comment

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

Yes, so there are two reasons here:

  1. This condition will not occur since we are initialising this through our own Laminator contract
  2. Even if we do manage to break this, it is initialized using create2 code block which does not revert and emits a ProxyCreated event anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that's fair, thanks.

Copy link
Contributor

@xBalbinus xBalbinus left a comment

Choose a reason for hiding this comment

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

Lgtm except for the removed checks on the setters, lmk if that's repetitive and we can merge.

@xBalbinus xBalbinus merged commit 0acd2c1 into main May 28, 2024
3 checks passed
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.

2 participants