-
Notifications
You must be signed in to change notification settings - Fork 135
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
chore: bump ethers #1242
chore: bump ethers #1242
Conversation
@mirceanis I have a hard time finding out whether these test results are really showing an error in the system. The generated signatures mismatch from the expected ones, but maybe that is ok as the content changed with the bigint usage? These are the only tests currently failing. Now waiting for release of ethr-did-resolver and ethr-did. |
I merge next into the branch and switched to the official releases of the 'ethr-did-resolver' & 'ethr-did'. Ready for review! |
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.
Overall pretty good but some small adjustments are still needed.
Thanks for powering through!
I figured out why the tests are failing there. ethers infers the transaction type when serializing it. v6 does the inference a little differently and ends up serializing transactions that specify a gasPrice as type 1 where v5 would infer type 0. Both transaction forms would be accepted by the EVM so it is ok to update the test vectors with the new values. For completeness, we should also have a test that sets the transaction type to 0 and compares with the previous test vectors: it('should sign EthTX using generic signer and specific type', async () => {
const transaction = Transaction.from({
to: '0xcE31a19193D4b23F4E9D6163d7247243BAF801C3',
value: 300000,
gasLimit: 43092000,
gasPrice: 20000000000,
nonce: 1,
type: 0, // enforce legacy serialization
})
const txData = transaction.unsignedSerialized
const rawTx = await agent.keyManagerSign({
algorithm: 'eth_signTransaction',
data: txData,
encoding: 'hex',
keyRef: importedKey.kid,
})
expect(rawTx).toEqual(
'0xf869018504a817c800840291882094ce31a19193d4b23f4e9d6163d7247243baf801c3830493e0801ba0f16e2206290181c3feaa04051dad19089105c24339dbdf0d80147b48a59fa152a0770e8751ec77ccc78e8b207023f168444f7cfb67055c55c70ef75234458a3d51',
)
}) |
Ahh thanks for spotting that. Amazing! I pushed the changes including the new test now. Let's see what the CI/CD says. At least on my local machine everything is green |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## next #1242 +/- ##
==========================================
- Coverage 84.99% 84.91% -0.09%
==========================================
Files 167 167
Lines 18001 18031 +30
Branches 2009 2017 +8
==========================================
+ Hits 15300 15311 +11
- Misses 2701 2720 +19
☔ View full report in Codecov by Sentry. |
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 good.
There are plenty of changes not covered by tests, but I think the original code paths weren't covered either.
// TODO: Make sure this works as we removed the options from arrayify | ||
if (data && data.substring(0, 2) !== "0x") { | ||
data = "0x" + data; | ||
} | ||
dataBytes = getBytes(data) |
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.
// TODO: Make sure this works as we removed the options from arrayify | |
if (data && data.substring(0, 2) !== "0x") { | |
data = "0x" + data; | |
} | |
dataBytes = getBytes(data) | |
dataBytes = hexToBytes(data) |
BREAKING CHANGE: now using ethers v6 as a dependency which may need extra attention when merging. The output of `eth_signTransaction` algorithms may be slightly different as transactions are by default infered as type 1 (EIP1559)
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
* chore(deps): bump ethers (#1242) * fix(deps): patch tests and package.json files after the ethers bump Co-authored-by: Dennis von der Bey <dennis@vonderbey.eu>
Still have to fix tests...