Skip to content
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

Merged
merged 26 commits into from
Oct 5, 2023
Merged

Conversation

fermentfan
Copy link
Contributor

Still have to fix tests...

@fermentfan fermentfan marked this pull request as ready for review September 18, 2023 13:18
@fermentfan
Copy link
Contributor Author

@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?

image

These are the only tests currently failing. Now waiting for release of ethr-did-resolver and ethr-did.

@fermentfan
Copy link
Contributor Author

I merge next into the branch and switched to the official releases of the 'ethr-did-resolver' & 'ethr-did'. Ready for review!

Copy link
Member

@mirceanis mirceanis left a 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!

__tests__/localAgent.test.ts Outdated Show resolved Hide resolved
__tests__/shared/didManager.ts Show resolved Hide resolved
__tests__/shared/ethrDidFlowSigned.ts Outdated Show resolved Hide resolved
__tests__/utils/ethers-provider.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/kms-local/src/key-management-system.ts Outdated Show resolved Hide resolved
packages/kms-local/src/key-management-system.ts Outdated Show resolved Hide resolved
packages/kms-local/src/key-management-system.ts Outdated Show resolved Hide resolved
packages/kms-web3/package.json Outdated Show resolved Hide resolved
packages/test-react-app/src/veramo/setup.ts Outdated Show resolved Hide resolved
@mirceanis
Copy link
Member

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',
        )
      })

@fermentfan
Copy link
Contributor Author

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
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (93198bc) 84.99% compared to head (a6a796f) 84.91%.
Report is 1 commits behind head on next.

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     
Files Coverage Δ
packages/did-provider-key/src/key-did-provider.ts 76.92% <100.00%> (ø)
packages/did-provider-pkh/src/pkh-did-provider.ts 62.72% <100.00%> (ø)
packages/key-manager/src/types.ts 100.00% <100.00%> (ø)
packages/kms-local/src/secret-box.ts 95.65% <100.00%> (-0.10%) ⬇️
...ackages/kms-web3/src/web3-key-management-system.ts 68.38% <100.00%> (-0.24%) ⬇️
packages/utils/src/did-utils.ts 83.56% <100.00%> (-0.05%) ⬇️
packages/did-provider-ion/src/functions.ts 94.13% <50.00%> (-0.02%) ⬇️
packages/key-manager/src/key-manager.ts 93.61% <80.00%> (-0.06%) ⬇️
packages/kms-local/src/key-management-system.ts 90.32% <95.45%> (+0.10%) ⬆️
.../key-manager/src/abstract-key-management-system.ts 61.01% <12.50%> (-6.26%) ⬇️
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mirceanis mirceanis left a 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.

Comment on lines +34 to +38
// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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)

@mirceanis mirceanis merged commit fbf5c69 into decentralized-identity:next Oct 5, 2023
9 of 11 checks passed
mirceanis pushed a commit that referenced this pull request Oct 23, 2023
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)
mirceanis added a commit that referenced this pull request Oct 24, 2023
* 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>
mirceanis added a commit that referenced this pull request Oct 24, 2023
* 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>
mirceanis added a commit that referenced this pull request Oct 24, 2023
* 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>
mirceanis added a commit that referenced this pull request Oct 24, 2023
* 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>
mirceanis added a commit that referenced this pull request Oct 24, 2023
* 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>
mirceanis added a commit that referenced this pull request Oct 24, 2023
* 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>
mirceanis added a commit that referenced this pull request Oct 25, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants