Skip to content

[SDK] Add ERC6551 Interface #1324

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

Merged
merged 27 commits into from
Oct 24, 2023
Merged

[SDK] Add ERC6551 Interface #1324

merged 27 commits into from
Oct 24, 2023

Conversation

ciaranightingale
Copy link
Contributor

No description provided.

@ciaranightingale ciaranightingale added DO NOT MERGE This pull request is still in progress and is not ready to be merged. CLI CLI-related changes labels Jul 12, 2023
@ciaranightingale ciaranightingale requested a review from a team July 12, 2023 14:10
@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2023

🦋 Changeset detected

Latest commit: c9616ed

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

This PR includes changesets to release 8 packages
Name Type
@thirdweb-dev/wallets Patch
@thirdweb-dev/auth Patch
thirdweb Patch
@thirdweb-dev/react-core Patch
@thirdweb-dev/react-native Patch
@thirdweb-dev/react Patch
@thirdweb-dev/unity-js-bridge Patch
@thirdweb-dev/react-native-compat 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

@ciaranightingale ciaranightingale marked this pull request as draft July 12, 2023 14:11

static id = walletIds.tokenBoundSmartWallet;
public get walletName() {
return "Token Bound Smart Wallet" as "Smart Wallet";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the as part? Was there a type error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, type error

constructor(options: WalletOptions<SmartWalletConfig>) {
super(SmartWallet.id, {
constructor(options: WalletOptions<SmartWalletConfig>, id?: string) {
super(id || SmartWallet.id, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, instead of adding a new id params, you can use the walletId param in the options object. You can change this code to be:

super(options.walletId || SmartWallet.id, {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to reflect these changes as well in token-bound-smart-wallet.ts:

constructor(options: WalletOptions<TokenBoundSmartWalletConfig>) {
        options.walletId = walletIds.tokenBoundSmartWallet;
        super(options
        );
    }

}

async initialize(personalWallet: EVMWallet) {
const factoryAddress = this.config.factoryAddress || "0x02101dfB77FDE026414827Fdc604ddAF224F0921";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this default value since users need to pass a factoryAddress here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they don't pass a factory address, we want them to use the ERRC-6551 official registry (this makes it a token bound account as they have claimed the term)


async initialize(personalWallet: EVMWallet) {
const factoryAddress = this.config.factoryAddress || "0x02101dfB77FDE026414827Fdc604ddAF224F0921";
await super.initialize(personalWallet, factoryAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the factoryAddress be changed once you do new TokenBoundConnnector ? If it cannot be changed then there's no need to pass this extra factoryAddress param here; initialize will just use whatever it was passed in the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behaviour I was trying to achieve here was to allow users to pass a factory address if they prefer but to provide the 6551 official address by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the same format as bundlerUrl etc

return account.prepare("executeCall", [target, value, data]);
},
getNonce: async (account) => {
return account.call("nonce", []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question but in the parent SmartWallet class, the calls here are: execute and getNonce, just making sure these are different on purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah in the official registry, the function signatures are different - they are of this format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you double check that these can be configured back if they so choose

@joaquim-verges could you check this please - my assumption was to use the erc-6551 registry function signatures by default rather than the ones we have - is this the route we are choosing to go? SHould we have a custom option or something which uses the thirdweb defaults? I just wanted to keep things simple but I'm not sure

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

see 138 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@ciaranightingale ciaranightingale requested a review from iketw July 26, 2023 19:18
@ciaranightingale ciaranightingale removed the CLI CLI-related changes label Jul 27, 2023
@joaquim-verges
Copy link
Member

/release-pr

@ciaranightingale ciaranightingale marked this pull request as ready for review September 15, 2023 16:15
@ciaranightingale
Copy link
Contributor Author

/release-pr

2 similar comments
@ciaranightingale
Copy link
Contributor Author

/release-pr

@nkrishang
Copy link
Contributor

/release-pr

@joaquim-verges joaquim-verges requested a review from a team as a code owner October 14, 2023 04:05
@joaquim-verges joaquim-verges removed the DO NOT MERGE This pull request is still in progress and is not ready to be merged. label Oct 24, 2023
@joaquim-verges joaquim-verges added this pull request to the merge queue Oct 24, 2023
Merged via the queue into main with commit 4cb6e28 Oct 24, 2023
@joaquim-verges joaquim-verges deleted the ciara/tba branch October 24, 2023 04:55
@github-actions github-actions bot mentioned this pull request Oct 24, 2023
IDubuque pushed a commit that referenced this pull request Oct 31, 2023
Co-authored-by: ikethirdweb <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>
IDubuque pushed a commit that referenced this pull request Nov 9, 2023
[RN] Update weights (#1817)

[RN] i18n more strings and fix fontFamily (#1818)

[RN] i18n strings (#1819)

[RN] Allow custom fontFamily (#1820)

[SmartWallet] implement multidimensional nonces for smart wallets (#1821)

[wallets, react] expose `onAuthSuccess` callback in `paperWallet` (#1815)

[SDK] Perf improvement for erc-1155-signature-mintable (#1824)

[wallets, react] Add OKX wallet (#1826)

Version Packages (#1793)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

[SDK] Improve contract-roles methods (#1798)

Spicy chain gas override (#1805)

Use BigInt function instead of bigint primitive numbers (#1830)

[RN] Allows for custom auth with embeddedWallet (#1806)

[RN] Update changelog (#1833)

[SDK] Perf improvements for erc-20 methods (#1814)

Signed-off-by: Kien Ngo <[email protected]>

[SDK] Add ERC6551 Interface (#1324)

Co-authored-by: ikethirdweb <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>

Patch: missing data arg in isSmartWalletDeployed (#1840)

[RN] Update naming for embeddedWallet jwt flow (#1841)

[SDK] Remove barrel files in `@r\packages\sdk\src\evm` (#1827)

Signed-off-by: Kien Ngo <[email protected]>

[RN] Migrate embedded wallet to new api (#1844)

[SDK] Fix double trailing slash when downloading merkle data (#1850)

chore(chains): sync chains (#1843)

Signed-off-by: Jonas Daniels <[email protected]>

[Wallet] Fix getSmartWalletAddress util function (#1851)

[wallets] - Fix chains package not tree-shaken because of SmartWallet  (#1853)

[SDK/Wallets] Override crypto-js to use latest version to fix vulnera… (#1854)

Fix typo in comments (#1839)

Signed-off-by: vuittont60 <[email protected]>

feat: Update minimum required Node version to 18.17.0 (#1855)

[SmartWallet] Expose new estimation functions for smart wallet transactions (#1856)

feat: Update minimum required Node.js version to >=18 (#1858)

Revert "[SDK/Wallets] Override crypto-js to use latest version to fix vulnera…" (#1859)

Version Packages (#1828)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

chore(chains): sync chains (#1865)

[SmartWallet] expose data in smart wallet utils (#1867)

Version Packages (#1866)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

[WALLET + REACT] Add Cometh Connect (#1749)

Signed-off-by: Manan Tank <[email protected]>
Co-authored-by: Manan Tank <[email protected]>
Co-authored-by: Joaquim Verges <[email protected]>

Revert "[WALLET + REACT] Add Cometh Connect" (#1879)

[SDK] Lazy load JSON files (#1862)

Signed-off-by: Kien Ngo <[email protected]>

[RN] Adds Japanese and Spanish support (#1878)
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.

4 participants