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: mark function visibility and apply formatting #455

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0xObsidian
Copy link

@0xObsidian 0xObsidian commented Dec 27, 2024

Motivation

This PR is to:

  • Prevent accidental state modifications
  • Make code intentions clearer
  • Enable better compiler optimizations for read-only calls

Change

  • Added pure/view modifiers to functions that don't modify
  • Applied forge fmt for consistent code formatting
  • Fixed compiler warnings related to state mutability
  • Updated gas snapshots to reflect current repo

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review

Additional Context

Breaking changes

  • No introduced changes

Testing

Fetch this commit branch and run

forge coverage -vvv

PR-Codex overview

This PR primarily focuses on improving code readability by formatting function signatures across various files. It enhances clarity without altering the logic or functionality.

Detailed summary

  • Reformatted function signatures to multi-line for better readability in multiple files.
  • Updated functions in IMigration.sol, Bundler.sol, IEIP712.sol, and others.
  • Modified internal functions to include view where applicable in test files.
  • Ensured consistent formatting for constructors and parameter lists.

The following files were skipped due to too many changes: src/StorageRegistry.sol, test/KeyRegistry/KeyRegistry.migration.t.sol, test/IdRegistry/IdRegistry.migration.t.sol, test/StorageRegistry/StorageRegistry.t.sol, .gas-snapshot

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

The setStorageRegistry function was missing validation checks
for the new address, which could lead to setting invalid contracts
or non-contract addresses as the storage registry.

Description
----------
- Added zero address check using revert
- Added contract existence check
- Used revert with custom errors instead of require() for:
  - Gas efficiency (4-byte selector vs string storage)
  - Better ABI compatibility via custom errors
- Updated registry tests to align with the current implementation
- Fixed `assumeNotPrecompile` issue
- Ran forge formatting on all modified files in this PR scope

Testing the fix
----------------
Fetch this PR branch and from the root:

Add `foundry-rs/forge-std`
```
forge install foundry-rs/forge-std
```

Run test
```
forge test --match-test testFuzzSetStorageRegistry -vvv

```
Description
-----------
- Added pure/view modifiers to functions that don't modify state
- Applied forge fmt for consistent code formatting
- Fixed compiler warnings related to state mutability

Reason
------
- To prevent accidental state modifications
- To make code intentions clearer
- To enable better compiler optimizations for read-only calls

Breaking changes
----------------
- No introduced changes

Testing
--------
Fetch this commit branch and run
```
forge coverage -vvv
```
Gas snapshots should reflect current changes
from adding proper state mutability modifiers.
@0xObsidian 0xObsidian changed the title 0x obsidian/mark function visibility and apply formatting fix: mark function visibility and apply formatting Dec 27, 2024
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.

1 participant