-
Notifications
You must be signed in to change notification settings - Fork 196
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
Fix signMessage and test against ganache #26
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
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.
Looks like a good base to further improve the handling of the signatures in the future.
E.g. check also if it is a contract signature (would be part of EIP-1271 issue)
const recoveredAddress = bufferToHex(pubToAddress(recoveredData)) | ||
hasPrefix = !sameString(recoveredAddress, ownerAddress) | ||
} catch (e) { | ||
hasPrefix = true |
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.
Wouldn't it make more sense to throw? E.g. if we cannot recover it, something is probably wrong
v: parseInt(signature.slice(130, 132), 16) | ||
} | ||
const recoveredData = ecrecover( | ||
Buffer.from(txHash.slice(2), 'hex'), |
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.
Should we check this also for the prefixed hash and then throw if it matches neither? In the future we could this way also properly handle contract signatures
@@ -11,6 +11,7 @@ | |||
], | |||
"scripts": { | |||
"build": "yarn rimraf dist && hardhat compile && tsc", | |||
"test:ganache": "export TEST_NETWORK=ganache && hardhat --network localhost deploy && nyc hardhat --network localhost test", |
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.
Should we run this in the CI?
|
||
async function getGanacheAccounts(): Promise<Account[]> { | ||
const web3 = new Web3('http://localhost:8545') | ||
const provider = new ethers.providers.Web3Provider(web3.currentProvider as any) |
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.
why any?
tests/utils/setupConfig.ts
Outdated
const provider = new ethers.providers.Web3Provider(web3.currentProvider as any) | ||
const accounts: Account[] = [] | ||
for (let i = 0; i < 10; i++) { | ||
const signer: Signer = provider.getSigner(i) |
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.
aren't types inherited?
signMessage
it checks if the message had a prefix or not to adjust thev
value accordinglyyarn test:ganache
to run the tests against ganache optionally