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

AA-345: Change "magic" size to 20 bytes as a placeholder for future aggregation RIP #8

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

drortirosh
Copy link

No description provided.


abi.encode(context, MAGIC_VALUE_PAYMASTER, validUntil, validAfter)

MAGIC_VALUE_PAYMASTER <20 bytes>, validUntil <6 bytes>, validAfter <6 bytes>, 64 <constant, 32 bytes>, length <32 bytes>, context <variable>

Choose a reason for hiding this comment

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

I think at this point there is no benefit in "truncating" the magic value to 20 bytes (because we expect an address there sometime in the future) and we should just give it 32 bytes. Makes it much easier to return the values in Solidity.

Copy link
Author

Choose a reason for hiding this comment

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

at this point all wallets are 4337, which means they already do it.
we either returns( bytes32 validationData ) (which is a packing of after/until/magic)
or returns (uint256 magic, uint256 validAfter, uint256 validUntil)
(and an extra bytes context for paymaster)

Choose a reason for hiding this comment

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

returns (uint256 magic, uint256 validAfter, uint256 validUntil) (and an extra bytes context for paymaster)

I think this would be much better and won't actually cost more than the packed version. I strongly suggest we consider doing exactly this and returning each variable with a native 256-bit value instead of shifting, packing and so on.

RIPS/rip-7560.md Outdated Show resolved Hide resolved

Same as in the [`sender` validation frame](#sender-validation-frame), in order to support gas estimation this
frame should return a value different from `MAGIC_VALUE_PAYMASTER` for conditions that cannot be satisfied
before signing.
frame should revert on errors, but return `MAGIC_VALUE_SIGFAIL` if the failure is because of signing error.

Choose a reason for hiding this comment

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

"in order to support gas estimation this frame should return MAGIC_VALUE_SIGFAIL if the failure is only caused by a signature verification error. This frame can revert in case of other exceptions." Something like that?

Choose a reason for hiding this comment

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

I still think this phrase could be rephrased to be clearer.

Copy link
Author

Choose a reason for hiding this comment

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

then please rephrase.

@forshtat forshtat changed the title return values AA-345: Change "magic" size to 20 bytes as a placeholder for future aggregation RIP Jun 25, 2024
@drortirosh drortirosh merged commit a4925da into master Jun 26, 2024
@drortirosh drortirosh deleted the return-values branch June 27, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants