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

formatting: First round of cleanup #21

Merged
merged 8 commits into from
Jun 21, 2024
Merged

formatting: First round of cleanup #21

merged 8 commits into from
Jun 21, 2024

Conversation

lucas-manuel
Copy link
Contributor

@lucas-manuel lucas-manuel commented Jun 13, 2024

Updates

  • General formatting
  • Update to solc 0.8.22 to remove unchecked { ++i }
  • Remove redundant getters in favour of public state variables
  • Update natspec formatting
  • Abstract away repeated code in _cancelTransaction and _executeTransaction to _removeActionFromQueue
  • Use early exit returns where possible
  • Use ternary operators where possible
  • Do some if statement formatting to improve readability
  • Add section labels
  • Reorder some view functions
  • Update _actionsSetCounter to actionsSetCount so it makes more sense as a public function
  • Update _queuedActions to isActionQueued and get rid of redundant isActionQueued view function

Open Questions

What should we say instead of misc functions? Do we need executeDelegateCall anymore?

@lucas-manuel lucas-manuel self-assigned this Jun 13, 2024
@lucas-manuel lucas-manuel changed the title feat: Update with new cleanup formatting: Update with new cleanup Jun 13, 2024
@lucas-manuel lucas-manuel changed the title formatting: Update with new cleanup formatting: First round of cleanup Jun 13, 2024
Base automatically changed from SC-472-guardian-merge-into-access-control to SC-463-merge-auth-bridge June 14, 2024 07:06
Base automatically changed from SC-463-merge-auth-bridge to master June 14, 2024 07:07
Copy link

Coverage after merging first-cleanup into master will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   Executor.sol100%100%100%100%

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

LGTM. executeDelegateCall is needed to supply an ETH msg.value for a delegate call.

@lucas-manuel lucas-manuel merged commit 9374feb into master Jun 21, 2024
3 checks passed
@lucas-manuel lucas-manuel deleted the first-cleanup branch June 21, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants