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
Add tests for EIP-1271 #416
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Pull Request Test Coverage Report for Build 4145622304
💛 - Coveralls |
I created a PR #499 that adds support for smart contract signatures in |
ccc287c
to
e00f4d8
Compare
data: "", | ||
dynamic: true, | ||
}; | ||
const signature = buildSignatureBytes([signerSafe1,emptySignSafe2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikhailxyz, emptySignSafe2
was created to include the address of safe2 in the staticpart
.
e00f4d8
to
eb001ad
Compare
|
||
it('should use EIP-1271 (contract signatures)', async () => { | ||
const { safe1, safe2, parentSafe, handlerSafe1, handlerSafe2, staticPart} = await setupTests() | ||
// Deposit some spare money for execution to parent safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little puzzled by the comment because technically smart contract wallets cant execute anything, would rename it for the test transfer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look to b677115
await expect(await hre.ethers.provider.getBalance(parentSafe.address)).to.be.deep.eq(parseEther("0")) | ||
}) | ||
|
||
it('should revert with hash not approved ', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary space in the test description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed b677115
const nonce = await parentSafe.nonce() | ||
const messageData = await parentSafe.encodeTransactionData(to, value, data, operation, 0, 0, 0, AddressZero, AddressZero, nonce) | ||
|
||
// Get hash transaction for each safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Get hash transaction for each safe | |
// Get message hash for each safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done b677115
also the test seems to be not linted, you can run |
Modify comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but i'll leave the final decision to @rmeissner
Closes #409