-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
adapt to eip-1193 provider changes #170
Conversation
|
||
// deploy log-echo contract | ||
const coinbase = await sendAsync({ method: 'eth_coinbase' }) | ||
const { contractAddress } = await deployLogEchoContract({ tools, from: coinbase }) | ||
const accounts = await request({ |
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.
It was throwing Error: Returned error: sender account not recognized
, when coinbase (0x000....) supplied as a from account. So coinbase
account has been replaced with first account from the accounts array.
|
||
// deploy log-echo contract | ||
const coinbase = await sendAsync({ method: 'eth_coinbase' }) | ||
const { contractAddress } = await deployLogEchoContract({ tools, from: coinbase }) | ||
const accounts = await request({ |
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.
It was throwing Error: Returned error: sender account not recognized
, when coinbase (0x000....) supplied as a from account. So coinbase
account has been replaced with first account from the accounts array.
method: 'eth_sendTransaction', | ||
params: { | ||
from: coinbase, | ||
params: [{ |
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.
It was throwing Error: Incorrect number of arguments. 'eth_sendTransaction' requires exactly 1 argument
error without the []
.
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
@@ -52,8 +55,14 @@ | |||
"ethereumjs-util>ethereum-cryptography>keccak": false, | |||
"ethereumjs-util>ethereum-cryptography>secp256k1": false, | |||
"ethjs-query>babel-runtime>core-js": false, | |||
"ganache-cli>ethereumjs-util>ethereum-cryptography>keccak": false, | |||
"ganache-cli>ethereumjs-util>ethereum-cryptography>secp256k1": false | |||
"ganache>keccak": false, |
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.
New ganache config has been added to fix.
@lavamoat/allow-scripts has detected dependencies without configuration. explicit configuration required.
run "allow-scripts auto" to automatically populate the configuration.
packages missing configuration:
- ganache>@trufflesuite/bigint-buffer [1 location(s)]
- ganache>@trufflesuite/uws-js-unofficial>bufferutil [1 location(s)]
- ganache>@trufflesuite/uws-js-unofficial>utf-8-validate [1 location(s)]
- ganache>bufferutil [1 location(s)]
- ganache>leveldown [1 location(s)]
- ganache>utf-8-validate [1 location(s)]
error Command failed with exit code 1.
package.json
Outdated
"sinon": "^15.2.0", | ||
"tape": "^5.7.0" | ||
}, | ||
"resolutions": { | ||
"@metamask/eth-json-rpc-provider": "^4.1.0" |
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.
Can this be done without relying on resolutions
? As this package does not have dependencies bundled, it means the desired version will not be used by users/dependencies of this package as intended.
(Unless this is purely for the sake of transitive devDependency for tests, in which case OK for now i guess?)
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.
Sure, removed the resolutions from package.json.
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've to revert it and bring back the resolutions
.
Yes, it is for the tests. Even before this PR, we are using @metamask/eth-json-rpc-provider
package in the tests without implicitly specifying in the package.json (which will be supplied by @metamask/eth-block-tracker
). The request
method is available only from 4.1.0 of @metamask/eth-json-rpc-provider
. Without the resolutions, I see tests failing for request
method not available.
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.
It seems like there is an undeclared peerDependencies
requirement somewhere, seeing that @metamask/eth-json-rpc-middleware
v13 seems necessary be a new requirement after this change and v12 and lower break...?
Would it be possible to introduce these changes in a non-breaking way which are still compatible with the non-latest versions of the peer-packages?
(Tests pass without resolutions
when bumping @metamask/eth-json-rpc-middleware
to 13)
- remove resolutions - yarn deduplicate
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!
@SocketSecurity ignore npm/async@2.6.4 |
After
SafeEventEmitterProvider
is updated to support EIP-1193 and a new version of@metamask/eth-json-rpc-provider
is released, we should adapt to the changes:sendAsync
deprecated method has been replaced withrequest
method.Fixes #151