Skip to content

Conversation

@adam-alchemy
Copy link
Contributor

Motivation

The purpose of the reference implementation should be to provide a readable and easy to comprehend implementation of the standard. Optimizations are useful for performance and necessary in production implementations of modular accounts and plugins, but impede readability of the reference implementation.

Specifically, we have lots of unchecked loop increments adding to SLOC without contributing to core account functionality. These are also automatically set to be unchecked in most cases starting with solidity v0.8.22.

Solution

Remove unchecked loop increments.

Bump solc to v0.8.25.

Remove an unused custom error type.

@adam-alchemy adam-alchemy requested a review from howydev April 18, 2024 16:40
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

Nice!

unchecked {
--i;
}
// Decrement here, instead of in the loop body, to handle the case where length is 0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be a bit more clear, this is to be able to index into postHooksToRun[i] below instead of postHooksToRun[i-1]. Very minor optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. I tried setting the starting value of i to postHooksToRunLength - 1 at first, but this causes an underflow at length = 0, so I added this comment to indicate usage.

@adam-alchemy adam-alchemy merged commit 8b58330 into v0.8-develop Apr 18, 2024
@adam-alchemy adam-alchemy deleted the adam/remove-opts-readability branch April 18, 2024 18:00
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.

4 participants