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

EIP-5792 Support #3001

Merged
merged 34 commits into from
May 16, 2024
Merged

EIP-5792 Support #3001

merged 34 commits into from
May 16, 2024

Conversation

gregfromstl
Copy link
Member

@gregfromstl gregfromstl commented May 11, 2024

PR-Codex overview

This PR focuses on enhancing wallet capabilities and improving random bytes generation.

Detailed summary

  • Updated randomBytes32 function to accept a length parameter for flexible byte generation.
  • Added OneOf type utility for handling optional object keys.
  • Improved wallet capabilities checking for smart and in-app wallets.
  • Refactored random bytes generation in various ERC token signature functions.

The following files were skipped due to too many changes: packages/thirdweb/src/extensions/erc4337/account/common.ts, pnpm-lock.yaml, packages/thirdweb/src/wallets/wallet-connect/index.ts, packages/thirdweb/src/wallets/eip5792/types.ts, .changeset/eight-planes-design.md, packages/thirdweb/src/wallets/injected/index.ts, packages/thirdweb/src/wallets/eip5792/show-calls-status.test.ts, packages/thirdweb/src/wallets/eip5792/show-calls-status.ts, packages/thirdweb/src/wallets/eip5792/get-calls-status.ts, packages/thirdweb/src/wallets/eip5792/get-capabilities.ts, packages/thirdweb/src/wallets/in-app/core/lib/in-app-wallet-calls.ts, packages/thirdweb/src/wallets/coinbase/coinbaseSDKWallet.ts, packages/thirdweb/src/wallets/eip5792/get-calls-status.test.ts, packages/thirdweb/src/wallets/eip5792/send-calls.ts, packages/thirdweb/src/wallets/eip5792/get-capabilities.test.ts, packages/thirdweb/src/wallets/eip5792/send-calls.test.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

linear bot commented May 11, 2024

Copy link

changeset-bot bot commented May 11, 2024

🦋 Changeset detected

Latest commit: b80aaca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
thirdweb Minor
@thirdweb-dev/sdk Patch
@thirdweb-dev/cli Patch
@thirdweb-dev/react-core Patch
@thirdweb-dev/react Patch
@thirdweb-dev/unity-js-bridge Patch
@thirdweb-dev/wallets Patch
@thirdweb-dev/auth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codspeed-hq bot commented May 11, 2024

CodSpeed Performance Report

Merging #3001 will not alter performance

Comparing greg/cnct-1136 (b80aaca) with main (cfe60d2)

Summary

✅ 9 untouched benchmarks

Copy link

codecov bot commented May 11, 2024

Codecov Report

Attention: Patch coverage is 81.63017% with 151 lines in your changes are missing coverage. Please review.

Project coverage is 61.93%. Comparing base (cfe60d2) to head (b80aaca).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3001      +/-   ##
==========================================
- Coverage   62.38%   61.93%   -0.45%     
==========================================
  Files         751      765      +14     
  Lines       52235    54164    +1929     
  Branches     2968     3039      +71     
==========================================
+ Hits        32588    33549     +961     
- Misses      19146    20112     +966     
- Partials      501      503       +2     
Flag Coverage Δ *Carryforward flag
legacy_packages 54.02% <ø> (ø) Carriedforward from 7346b9a
packages 63.73% <81.63%> (-0.64%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...sions/airdrop/write/airdropERC1155WithSignature.ts 100.00% <100.00%> (ø)
...ensions/airdrop/write/airdropERC20WithSignature.ts 100.00% <100.00%> (ø)
...nsions/airdrop/write/airdropERC721WithSignature.ts 100.00% <100.00%> (ø)
...s/thirdweb/src/extensions/erc1155/write/sigMint.ts 97.60% <100.00%> (ø)
...ges/thirdweb/src/extensions/erc20/write/sigMint.ts 95.78% <100.00%> (ø)
.../thirdweb/src/extensions/erc4337/account/common.ts 95.12% <100.00%> (ø)
...es/thirdweb/src/extensions/erc721/write/sigMint.ts 96.42% <100.00%> (ø)
...es/thirdweb/src/transaction/prepare-transaction.ts 100.00% <100.00%> (ø)
packages/thirdweb/src/utils/uuid.ts 100.00% <100.00%> (ø)
packages/thirdweb/src/wallets/create-wallet.ts 53.55% <100.00%> (+0.80%) ⬆️
... and 11 more

... and 5 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented May 11, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 39.08 KB (-0.13% 🔽) 782 ms (-0.13% 🔽) 2.6 s (+28.21% 🔺) 3.4 s
thirdweb (cjs) 88.12 KB (+0.1% 🔺) 1.8 s (+0.1% 🔺) 5.8 s (-1.5% 🔽) 7.5 s
thirdweb (minimal + tree-shaking) 4.73 KB (0%) 95 ms (0%) 320 ms (-45.01% 🔽) 415 ms
thirdweb/chains (tree-shaking) 400 B (0%) 10 ms (0%) 51 ms (-13.79% 🔽) 61 ms
thirdweb/react (minimal + tree-shaking) 16.49 KB (+0.09% 🔺) 330 ms (+0.09% 🔺) 416 ms (+16.44% 🔺) 746 ms

@gregfromstl gregfromstl added the DO NOT MERGE not ready to be merged yet label May 12, 2024
@gregfromstl gregfromstl removed the DO NOT MERGE not ready to be merged yet label May 13, 2024
@gregfromstl
Copy link
Member Author

/release-pr

@gregfromstl
Copy link
Member Author

/release-pr

.changeset/eight-planes-design.md Outdated Show resolved Hide resolved
.changeset/eight-planes-design.md Show resolved Hide resolved
Requests the wallet to show the status of a given bundle ID.

```ts
await showCallsStatus({ wallet, bundleId });
Copy link
Contributor

Choose a reason for hiding this comment

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

does this actually work? it opens the wallet with the current status?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works if the wallet supports 5792. I'm honestly not a big fan of it since it makes app developers more reliant on the wallet, but its in the spec so I included it.

packages/thirdweb/src/exports/wallets.ts Outdated Show resolved Hide resolved
packages/thirdweb/src/utils/encoding/hex.ts Outdated Show resolved Hide resolved
} from "../../../eip5792/types.js";
import type { Account, Wallet } from "../../../interfaces/wallet.js";

const bundlesToTransactions = new Map<string, Hex[]>();
Copy link
Contributor

Choose a reason for hiding this comment

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

this map could get huge in a long running app. would use a bounded map here or a LRU

/**
* @internal
*/
export async function inAppWalletSendCalls(args: {
Copy link
Contributor

Choose a reason for hiding this comment

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

having the wallet.sendCalls in the interface would def make it much cleaner here. separate implementation for inapp vs smart. this function does 2 jobs implicitely here (smart + inapp)

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be better to separate it anyway? They were originally separate but I combined them for reuse.

packages/thirdweb/src/wallets/in-app/core/wallet/index.ts Outdated Show resolved Hide resolved
@gregfromstl
Copy link
Member Author

/release-pr

@gregfromstl
Copy link
Member Author

/release-pr

@gregfromstl
Copy link
Member Author

/release-pr

@gregfromstl gregfromstl added this pull request to the merge queue May 16, 2024
Merged via the queue into main with commit 726618b May 16, 2024
15 checks passed
@gregfromstl gregfromstl deleted the greg/cnct-1136 branch May 16, 2024 18:58
@jnsdls jnsdls mentioned this pull request May 16, 2024
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