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

Integrate DAB with Stoic Wallet #13

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

tarek-eg
Copy link

@tarek-eg tarek-eg commented Dec 22, 2021

Link to forum
https://forum.dfinity.org/t/icdevs-org-bounty-integrate-dab-with-stoic-wallet-6-100-icp/9824

  • Have stoic wallet query getAllgetUserTokens in DAB and append those items to the ones that stoic already finds search for duplicates and remove them.
  • Keep track of if the NFT is in DAB and use DAB’s transfer function to call transfer if it is a DAB listed token.
  • Use DAB’s details function to get info and show it when a user clicks on an NFT in stoic wallet and wants to see details

@tarek-eg
Copy link
Author

Open questions:
Stoic collections has the following properties but DAB doesn't, what should we default to in these cases?

   comaddress ? 
   commission ?
   route ?

@tarek-eg tarek-eg force-pushed the integrate-dab branch 2 times, most recently from 17b4ed9 to ffa4d66 Compare December 23, 2021 15:00
@tarek-eg
Copy link
Author

tarek-eg commented Dec 26, 2021

An update on the progress so far.
Query DAB Nfts is working fine.
A little issue with the thumbnails in DAB, since the returned NFTS could be of many types we need to check first the content_type and based on the that we should know how to render, currently I am falling back to the collection icon.
A link to discussion on DAB discord

Transfer function:
I am a bit blocked on the transfer function since there's a mismatch between agent-js versions used by DAB-js and stoic that causing an error.
Stoic is using 0.8.9 and we need to upgrade to at least version 0.9.3 which has a breaking changes.
I will be spending sometime with the upgrade but the changes could grow out of proportion.

- required to be compatible with DAB-js
@tarek-eg tarek-eg marked this pull request as ready for review December 26, 2021 19:58
@tarek-eg
Copy link
Author

Ready for review

@stephenandrews
Copy link
Contributor

Thanks will review and test locally, thanks for this!

@stephenandrews
Copy link
Contributor

Open questions: Stoic collections has the following properties but DAB doesn't, what should we default to in these cases?

   comaddress ? 
   commission ?
   route ?

We will pull from a second API (entrepot specific) to pull these in - these are entrepot specific/marketplace variables, but soon will be stored in-canister

@stephenandrews
Copy link
Contributor

Tried to run locally and got the following, please advise:

./src/components/SendNFTForm.js
Module not found: Can't resolve '@dfinity/principal' in 'C:\Users\Stephen Andrews\Documents\GitHub\stoic-wallet\src\components'

@tarek-eg
Copy link
Author

/components/SendNFTForm.js

Can you delete your node_modules and reinstall again?
I suspect there's a conflict in @dfinity/principal versions.

@tarek-eg
Copy link
Author

tarek-eg commented Jan 18, 2022

Probably you will also need to follow this in order to install DAB
https://docs.dab.ooo/nft-list/getting-started/#0-preparing-your-environment

To pull and install from @Psychedelic via the NPM CLI, you'll need:

A Github account
A Github personal access token
The personal access token with the correct scopes, repo and read:packages to be granted access to the GitHub Package Registry.
Authentication via npm login, using your Github email for the username and the personal access token as your password:

Once you have those ready, run:
npm login --registry=https://npm.pkg.github.com --scope=@Psychedelic

@stephenandrews
Copy link
Contributor

Merged, built and tested locally - some issues:

  1. NFTs are displaying the wrong image - seems to be the same image for all NFTs of the same collection. Clicking the image does go to the right image though
  2. For some reason it is showing the wrong NFT count - I'm seeing the same number of NFTs for every wallet, which is wrong

@tarek-eg
Copy link
Author

tarek-eg commented Jan 23, 2022

Merged, built and tested locally - some issues:

  1. NFTs are displaying the wrong image - seems to be the same image for all NFTs of the same collection. Clicking the image does go to the right image though
  2. For some reason it is showing the wrong NFT count - I'm seeing the same number of NFTs for every wallet, which is wrong

For the first issue this happens because DAB NFTS can be of types other than image, in order to get the correct image we will have to make a http request for each image to figure out how to display.
@letmejustputthishere has a opened a PR that is targeting the same issue.

We can merge his PR after merging this one or I can integrate his PR in this one what do you think.

For the second issue how can we debug this?
Can you give more details on how to reproduce this? Is it different wallets with different identity or is it the same wallet with different account (sub account)?

@tarek-eg
Copy link
Author

I have fixed the thumbnail display issue, now it's showing the actual image for each NFT.

@tarek-eg
Copy link
Author

tarek-eg commented Jan 31, 2022

Merged, built and tested locally - some issues:

  1. For some reason it is showing the wrong NFT count - I'm seeing the same number of NFTs for every wallet, which is wrong

DAB connects NFTS to the principal id by using sub-account 0, so for each principal you will see the same NFT count for each subaccount, maybe that's what you meant or did you mean a different identity has the same NFT count?

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