zerodevapp / kernel

https://docs.zerodev.app/
MIT License
178 stars 63 forks source link

⚡️ Optimise gas usage when enabling p256 validator, add a few comments #52

Closed KONFeature closed 10 months ago

KONFeature commented 11 months ago
leekt commented 11 months ago

Makes sense on the sload stuff and the event issue.

@de33 do you want to comment on the usage of the oldKey?

leekt commented 11 months ago

@KONFeature i think we can just remove the oldKey from event. Can you add a commit for that?

KONFeature commented 11 months ago

@KONFeature i think we can just remove the oldKey from event. Can you add a commit for that?

Pushed :) Quick question, why the double signature validation during validateUserOp & validateSignature? And so why validating hash given has param & then rebuilt hash like the first hash was re hashed like a ethereum signed message? Since the p256 validation process is quite gas heavy, having this double validation, will cost a lot to the bundler / paymaster if the tx has invalid signature, since it would always imply to perform the check on both hash. I think the valid hash to check against is the non signed message one, but I can be wrong

leekt commented 11 months ago

Main purpose of this is allowing both eth signed/non signed message to achieve backward compatibility.

I do hope that we dont have to do it this way but there are still some dapps that has non eth-signed messages

If you have some opinion on this, i am ipen for discussion

KONFeature commented 11 months ago

Yeah, I think that make sense for the ecdsa validator since it's the most common way to sign a message, so the easiest way to integrate it, and the gas impact is not very high. But for the p256 signer, since the gas impact would be really huge, picking only one type of signature wanted is the way to go in my opinion. I think the use of the userOpHash directly, so without the ethereum sign message prefix, since it's adding some gas overhead also, would be better. If asked by community or other project, we can have two p256 signer deployed, one with eth sign mesasge & the other without it using direct userOpHash signature