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

[SDK] Fix invalid recipient issue when deploy built-in contracts #2020

Merged
merged 9 commits into from
Dec 15, 2023

Conversation

kien-ngo
Copy link
Contributor

@kien-ngo kien-ngo commented Nov 29, 2023

Problem solved

In the past we default platform_fee_recipient and primary_sale_recipient to AddressZero.
However due to a recent change in our smart contract extensions (PrimarySale & PlatformFee), AddressZero is no longer an accepted value for those fields.
So now we set the default value to the signer address.
ref: thirdweb-dev/contracts#530

Changes made

  • Public API changes: list the public API changes made if any
  • Internal API changes: explain the internal logic changes

How to test

  • Automated tests: link to unit test file
  • Manual tests: step by step instructions on how to test

@kien-ngo kien-ngo requested a review from a team November 29, 2023 11:19
Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 34ff60a

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

This PR includes changesets to release 9 packages
Name Type
@thirdweb-dev/sdk 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/wallets Patch
@thirdweb-dev/auth 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

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

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

Comparison is base (046603a) 67.48% compared to head (34ff60a) 67.49%.
Report is 18 commits behind head on main.

Files Patch % Lines
packages/sdk/src/core/utils/apiKey.ts 71.42% 1 Missing and 1 partial ⚠️
...kages/sdk/src/evm/core/classes/internal/factory.ts 87.50% 1 Missing ⚠️
packages/sdk/src/evm/core/sdk.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2020   +/-   ##
=======================================
  Coverage   67.48%   67.49%           
=======================================
  Files         291      292    +1     
  Lines       11019    11028    +9     
  Branches     1511     1514    +3     
=======================================
+ Hits         7436     7443    +7     
  Misses       2960     2960           
- Partials      623      625    +2     

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

@kien-ngo kien-ngo self-assigned this Nov 30, 2023
Copy link
Member

@joaquim-verges joaquim-verges left a comment

Choose a reason for hiding this comment

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

wonder if we can make primary_sale_recipient optional now with this? it's required right now in the deploy metadata objects

@kien-ngo
Copy link
Contributor Author

@joaquim-verges Hmm I'm not aware of any recent updates. Are you saying we don't need this PR?

@kien-ngo
Copy link
Contributor Author

Ah I see, you were talking about the primary_sale_recipient and not platform_fee_recipient. Yeah I suppose we can leave it out

@joaquim-verges
Copy link
Member

Oh I meant leave your changes, but make it optional in NFTDeployMetadata and co (default to address zero)

@kien-ngo kien-ngo force-pushed the kien/fix-invalid-recipient branch from 10f6d39 to efad0a0 Compare December 14, 2023 22:13
@kien-ngo
Copy link
Contributor Author

Ty for the feedback. I'm updating the code plus adding a new test file!

@kien-ngo
Copy link
Contributor Author

Added new test file here:

https://github.com/thirdweb-dev/js/blob/34ff60ac1a2acc9bfebde00dbeb9b91d64ecd5f3/packages/sdk/test/evm/recipient.test.ts

Also primary_sale_recipient is optional now. Pls have a look @joaquim-verges ty

@kien-ngo kien-ngo added this pull request to the merge queue Dec 15, 2023
Merged via the queue into main with commit fed1313 Dec 15, 2023
10 checks passed
@kien-ngo kien-ngo deleted the kien/fix-invalid-recipient branch December 15, 2023 03:11
@github-actions github-actions bot mentioned this pull request Dec 15, 2023
jnsdls pushed a commit that referenced this pull request Jun 19, 2024
* Fix verification check

* Fix check again
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.

2 participants