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.

Sign up for GitHub

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

Jump to bottom

feat: add upper limit to gas price in light user op hash #43

Merged
merged 8 commits into from
Aug 29, 2024
Merged

feat: add upper limit to gas price in light user op hash #43

merged 8 commits into from
Aug 29, 2024

Conversation

r0ohafza
Copy link
Collaborator

No description provided.

@r0ohafza r0ohafza requested a review from wminshew August 16, 2024 18:32
wminshew
wminshew approved these changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@wminshew wminshew left a comment

Choose a reason for hiding this comment

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

lgtm -- let's leave unmerged for now as we discussed earlier

i was originally thinking having each signer set their own limit, but this makes more sense. easy path to adding sig/op expiration if we want too

wminshew
wminshew reviewed Aug 26, 2024
View reviewed changes
packages/smart-vaults/src/vault/SmartVault.sol
/// in the light userOp hash to ensure last signer does not exceed the specified gas price/limits. These values will
/// be ignored when threshold is 1. paymaster, paymasterVerificationGasLimit and paymasterPostOpGasLimit will be
/// ignored if paymasterAndData is empty.
struct LightUserOpGasLimits {
Copy link
Contributor

Choose a reason for hiding this comment

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

vaguely tempted to pack a few of these to save the calldata but probably not worth it idk

Copy link

Choose a reason for hiding this comment

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

If packing is added, one suggestion would be to pack the callGasLimit and verificationGasLimit values into a bytes32 value called accountGasLimits, since that would match the PackedUserOperation struct itself

I think it'd also be possible to pack preVerificationGas and maxPriorityFeePerGas together, but that pairing would be a bit less natural and wouldn't match PackedUserOperation

@rholterhus
Copy link

These changes all look good to me, and I'll keep an eye on these PRs in case there are any other changes before being merged

Also one side note: originally I was thinking that each light signer would set their own upper-bound independently, but now that I'm looking at these changes, I see that wouldn't really work. Because the code only works if all the light signers agree on the same light hash, and if the upper bound doesn't contribute to the light hash, there'd need to be another set of signatures, which would be a big change and seems worse. And I don't think all the light signers needing to agree on the upper bound is even a problem anyway. So this all looks good to me

@r0ohafza r0ohafza merged commit 9e05ed6 into smart-vaults Aug 29, 2024
@r0ohafza r0ohafza deleted the max-gas-limit branch August 29, 2024 17:48
r0ohafza added a commit that referenced this pull request Sep 4, 2024
@r0ohafza @frangio
feat: Splits smart vaults (#51)
1c7fdc4 
* feat: create smart vaults package

* feat: add simple account and factory from eth-infitism/aa

* feat: restructure smart vault

* feat: restructure smart vault factory

* feat: multisig support with passkey and eoa signers

* feat: add root owner tests

* feat: add multi signer tests

* fix: signer cap

* feat: add factory tests

* feat: validateUserOp tests

* feat: execute tests

* feat: create functionality

* feat: avoid signing over gas userop until last signature

* chore: update docs

* Light state sync and multi user op (#28)

* feat: light state sync first pass

* feat: 1271 light state sync support

* fix: tests

* chore: light state sync tests

* chore: erc1271 tests

* feat: multiChain user op

* lil refactoring

* minor changes

* feat: light multi user op

* fix: rename to getX()

* fix: set nonce if and only if nonce is greater than storage

* fix: revert when trying to remove empty signer

* fix: minor fixes

* fix: capitalize immutables

* feat: rootOwner -> solay/Ownable

* fix: _nonce to _salt

* feat: add styling guide

* chore: gas report

* feat: don't return early in signature verification flows

* feat: add missing event in initialize flow

* fix: pass light hash directly

* feat: optimize isValidSignature flow

* fix: use constants

* fix: add constants to multiSignerLib

* chore: add more docs

* chore: add more initialization tests

* chore: add more user op signature tests

* chore: add more erc1271 signature tests

* feat: strip down light sync (#30)

* feat: strip down light sync

* fix: clean up assembly blocks

* fix: docs

* fix: address comments

* fix: address comment

* feat: remove onlySelfOrOwner

* docs: update signature scheme

* feat: add fallback module (#31)

* feat: add fallback module

* feat: add custom receive fn

* fix: remove code check when validating signer

* chore: rename bundle to merkelized

* fix: remove redundant brackets

* chore: remove via-ir for default profile

* feat: redo signature for user op and erc1271 (#33)

* feat: redo signature for user op and erc1271

* minor changes

* minor changes

* Refactor SmartVault (#34)

* fix compiler warning

* create AccountSigner and PasskeySigner

* move nonce logic to LightSync module

* use standard capitalization

* fix enum variant

* redesign signature of _validate functions

* deduplicate merkle logic

* remove redundant code

* deduplicate multisig logic

* reuse validation function

* merge multisig logic with and without in-memory updates

* feat: rip light sync

---------

Co-authored-by: Francisco <[email protected]>

* feat: operator manager (#32)

* feat: operator manager first pass

* feat: operator manager

* docs: minor update

* fix: address comments

* chore: operator -> module

* fix: tests

* fix: name

* chore: merge multiSignerSignatureLib to multiSignerLib

* fix: add comments in user op validation

* fix: minor doc updates

* chore: minor doc updates

* feat: update storage layout of signer (#36)

* feat: update storage layout of signer

* fix: minor fixes

* fix: minor fixes

* fix: address comments

* fix: address comments

* fix: using  MultiSignerLib

* fix: use global and remove unneeded private functions

* fix: use $.isValidSignature

* fix: update isPasskey logic

* fix: address will's comments

* docs: add security contact

* fix: add storage location fallback and module manager

* fix: change updateFallbackHandler modifier public -> external

* chore: add slither and remove dead code

* feat: multi signer auth (#38)

* feat: multi signer auth

* fix: docs:
:

* fix: add suggested doc change

* chore: update readme (#39)

* chore: update readme

* fix: address comments

* chore: add dummy passkey signature test

* fix: clean up multi signer auth (#40)

* fix: clean up multi signer auth

* feat: mapping to array

* fix: address comment

* feat: update _checkOwner() (#41)

* feat: add fallback handler called event (#42)

* chore: doc updates

* chore: doc updates

* test: add missing fallback test

* feat: add upper limit to gas price in light user op hash (#43)

* feat: add upper limit to gas price in light user op hash

* fix: update comment

* feat: add preVerification and callGas limits

* feat: add verification gas limit

* fix: docs

* feat: add paymaster limits

* fix: use paymasterVerificationGasLimit

* fix: ordering of params

* fix: prevent empty signers validating signature (#45)

* fix: revert when duplicate signer is used (#46)

* fix: revert when duplicate signer is used

* fix: move check one level out

* fix: verify light merkle root when threshold is > 1 (#47)

* fix: verify light merkle root when threshold is > 1

* fix: update doc

* feat: deployment script (#44)

* feat: custom webauthn (#50)

* chore: add audit report

* fix: lint

---------

Co-authored-by: Francisco <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rholterhus rholterhus rholterhus left review comments

@wminshew wminshew wminshew approved these changes

@mihoward21 mihoward21 mihoward21 left review comments

@frangio frangio frangio left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@r0ohafza @rholterhus @frangio @mihoward21 @wminshew