Safe BLS module #54
Safe BLS module #54
Conversation
| @@ -0,0 +1,101 @@ | |||
| // SPDX-License-Identifier: LGPL-3.0-only | |||
| pragma solidity >=0.7.0 <0.9.0; | |||
| pragma abicoder v2; | |||
There was a problem hiding this comment.
Seen that this is included in the SafeECDSAPlugin too. I believe abicoder was defaulted to v2 as of solidity 0.8.0. I think it makes sense here and in the ECDSA plugin as we're using solidity>=0.7.0. Curious if we want to keep supporting versions of solidity less than 0.8.0? Open question don't really have a strong opinion
There was a problem hiding this comment.
Sorry just getting around to these comments. Yeah, I don't have a preference either. I haven't really done a deep dive to know if it's still necessary to support the older versions
| UserOperation calldata userOp, | ||
| bytes32 userOpHash | ||
| ) internal view override returns (uint256 validationData) { | ||
| require(userOp.signature.length == 64, "VG: Sig bytes length must be 64"); |
There was a problem hiding this comment.
What do you think about custom errors as opposed to require statements with strings? https://soliditylang.org/blog/2021/04/21/custom-errors/
There was a problem hiding this comment.
Good call, I'll make the update
There was a problem hiding this comment.
Now that we are importing 4337 contracts, should we rename this to something more generic? Maybe something like TypesHelper?
| import { ethers } from "hardhat"; | ||
| import { expect } from "chai"; | ||
| import { AddressZero } from "@ethersproject/constants"; | ||
| import { utils } from "ethers-v5"; |
There was a problem hiding this comment.
Why do we need ethers-v5 here instead of v6?
| userWallet, | ||
| entryPoint, | ||
| safe, | ||
| safeProxyFactory, |
There was a problem hiding this comment.
If we're creating a safeProxyFactory variable and others before the describe block and then assigning to them, is there a reason to return them in this function as well?
Blocked by: #52
Adding a Safe module that uses BLS sigs to verify a transaction. I added a hardhat test that submits a userOp to the entrypoint directly. This way we don't need to run a bundler or start a node to test a module.
I'm wondering if we should reorganize our test directory. Right now we have three forms of tests.
yarn hardhat testand the tests should work. Hardhat tests are useful for verification types that are hard/not possible in solidity.forge test, and tests are written in solidity.Should rename/organise these differently? For example, the two ideas I have are:
test/hardhattest/forgetest/integrationor
test/hardhattest/hardhat/integration(since the integration tests use hardhat)test/forge