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/allow registry to be zero address #152

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

VGabriel45
Copy link
Collaborator

No description provided.

address(bootstrapper) == address(0) ||
address(registry) == address(0) ||
factoryOwner == address(0)),
!(implementation == address(0) || k1Validator == address(0) || address(bootstrapper) == 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 this is skipped too?
factoryOwner == address(0)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird, it was not removed on my local version

@@ -50,6 +50,29 @@ contract TestK1ValidatorFactory_Deployments is NexusTest_Base {
assertTrue(isContract(address(factory)), "Factory should be a contract");
}

/// @notice Tests that the constructor can take a zero address for the registry.
function test_ConstructorInitializesWithRegistryAddressZero() public {
Copy link
Contributor

Choose a reason for hiding this comment

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

we would also have 7484 tests somewhere with some mock registry.
we can set it to zero and see if everything is going fine.
cases I mentioned on slack thread once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure which tests you mean

Copy link
Contributor

Choose a reason for hiding this comment

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

pasting here @VGabriel45

we should have test cases for this with and without registry
not just mock registry but attesters and attestations being revoked and stuff

once it’s allowed zero everywhere it should function well when its set to 0
and set to non zero and tests i mentioned before
then set back to zero

cc @Aboudjem

Copy link

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

constable-states

Impact: Optimization
🔴 Confidence: High

base/RegistryAdapter.sol#L10

factory/RegistryFactory.sol#L39

_This comment was automatically generated by the GitHub Actions workflow._

Copy link

Changes to gas cost

Generated at commit: c1e212fbe98290171c3ee4dbe6bbdac4193619f0, compared to commit: 67d48082fb317198353e7310af80ecd3d74474fc

🧾 Summary (5% most significant diffs)

Contract Method Avg (+/-) %
Nexus execute +17,361 ❌ +54.86%
MockPaymaster getHash +41 ❌ +1.80%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
Nexus 5,641,328 (-158,392) execute
executeFromExecutor
initializeAccount
isModuleInstalled
validateUserOp
6,175 (-22)
14,477 (-22)
111,052 (0)
611 (0)
7,142 (0)
-0.36%
-0.15%
0.00%
0.00%
0.00%
49,006 (+17,361)
19,773 (-22)
130,887 (+15)
804 (-6)
8,713 (-29)
+54.86%
-0.11%
+0.01%
-0.74%
-0.33%
37,875 (+1,249)
19,603 (-22)
130,952 (0)
780 (0)
7,142 (0)
+3.41%
-0.11%
0.00%
0.00%
0.00%
142,953 (+95,013)
25,409 (-22)
130,952 (0)
2,839 (0)
35,933 (+119)
+198.19%
-0.09%
0.00%
0.00%
+0.33%
76 (+13)
4 (0)
309 (+58)
331 (+58)
351 (+63)
MockPaymaster 1,161,593 (0) getHash 2,087 (0) 0.00% 2,318 (+41) +1.80% 2,427 (+152) +6.68% 2,697 (+233) +9.46% 17 (+5)
Bootstrap 2,552,273 (0) initNexusScoped 62,207 (0) 0.00% 82,042 (+15) +0.02% 82,107 (0) 0.00% 82,107 (0) 0.00% 309 (+58)

Copy link
Contributor

@livingrockrises livingrockrises 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 to go. check comment about test cases. If done in different branch for coverage improvement then I can merge this

Base automatically changed from m_dev_cantina to dev August 27, 2024 07:10
@livingrockrises livingrockrises merged commit 5f7e359 into dev Aug 27, 2024
6 of 8 checks passed
@livingrockrises livingrockrises deleted the fix/allow_registry_to_be_zeroAddr branch August 27, 2024 07:22
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