-
Notifications
You must be signed in to change notification settings - Fork 44
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
Space id integration #1578
Space id integration #1578
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
All looks good. |
|
@mishramonalisha76 reopened this pull request. |
All looks good. |
@@ -44,6 +44,7 @@ | |||
"@testing-library/user-event": "^7.1.2", | |||
"@uniswap/widgets": "^2.47.3", | |||
"@unstoppabledomains/resolution": "8.5.0", | |||
"@web3-name-sdk/core": "^0.1.18", | |||
"@web3-onboard/coinbase": "^2.2.5", |
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.
@mishramonalisha76 the ticket description says resolved the domain name using space id package, but where is the space id package used?
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.
"@web3-name-sdk/core" this the space id package
} | ||
}); | ||
return ensName; | ||
const getDomainName = async (checksumWallet: string, setWeb3NameList: any) => { |
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.
If we have this functionality inside the sdk library then why not use it from there and avoid installing the space id library here?
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.
not sure why its not getting installed from the sdk itself, even though its mentioned in dependencies in uiweb package
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.
@mishramonalisha76 this needs a second approval from @corlard3y .
All looks good. |
The merge-base changed after approval.
The code provided has some issues:
Other than these issues, the code looks good. Please correct the mentioned issues above. If you have any other specific concerns or questions, feel free to ask. |
Pull Request Template
Ticket Number
Description
Type of Change
Checklist
Frontend Guidelines
Build & Testing
Screenshots/Video with Explanation
Before: Explain the previous behavior
After: What's changed now
Additional Context
Review & Approvals
Notes