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

NFT certificates are not visible in Explorer or Wallet #47

Closed
ryancwalsh opened this issue Mar 31, 2022 · 17 comments
Closed

NFT certificates are not visible in Explorer or Wallet #47

ryancwalsh opened this issue Mar 31, 2022 · 17 comments
Assignees
Labels
bug Something isn't working hard High Priority

Comments

@ryancwalsh
Copy link
Contributor

I minted a NCD cert for Rasha. I don't see it at https://explorer.near.org/accounts/rashaabdulrazzak.near or https://wallet.near.org/profile/rashaabdulrazzak.near .

But then again on testnet I did https://explorer.testnet.near.org/transactions/95XiTn7uEatwXU3D7Q2SrfTGbJYQ23UzMU69f1G1PW9E but don't see it in https://explorer.testnet.near.org/accounts/hatchet.testnet

How do NEAR NFTs work? Where should we be able to see them?
E.g. I think the NFT at least exists because https://certificates.near.university/account/rashaabdulrazzak.near shows it (the image is just broken because of a bug with canvas that I'll need to figure out due to Render using a different version of something).

@ryancwalsh ryancwalsh added the bug Something isn't working label Mar 31, 2022
@ryancwalsh
Copy link
Contributor Author

From Jacob:

I know why
https://nomicon.io/Standards/NonFungibleToken/Event
Mint event emission was added in pre.7
https://github.com/near/near-sdk-rs/blame/6596dc311036fe51d94358ac8f6497ef6e5a7cfc/near-contract-standards/src/non_fungible_token/core/core_impl.rs#L342
Upgrade the near-contract-standards package to
near-contract-standards = "4.0.0-pre.7"
try it out on testnet first

@ryancwalsh
Copy link
Contributor Author

@ryancwalsh
Copy link
Contributor Author

@encody I must have done c21628c#diff-2a7c850f07d7c32c9b9fc46f98c990db85465fbfbf71d17931951f59a3c9c108R65 wrong because after rebuild and redeploy and new mint, https://explorer.testnet.near.org/transactions/DDdGymM7EDRUkAaNECzDbYU1sLqtfoS2gtRfY8AzNZiU still doesn't show nft_mint event.

@ryancwalsh
Copy link
Contributor Author

ryancwalsh commented Mar 31, 2022

@encody Actually (as we guessed since you showed a screenshot proving that your testnet wallet does have some collectibles, so this has worked before), probably we can delete this whole branch. None of that new code should have been necessary.

I see self.tokens.internal_mint(token_id, to_account_id, Some(combined_metadata)) which then calls NftMint { owner_id: &token.owner_id, token_ids: &[&token.token_id], memo: None }.emit(); which looks like it should have taken care of it.

I just haven't figured out why that event hasn't been getting emitted.

@encody
Copy link
Contributor

encody commented Apr 12, 2022

I just did a fresh pull & build. It seems to work fine as-is: https://explorer.testnet.near.org/transactions/7a3BdXMGFprAG5n4rLVFvB6BEpu1VmUFQ9PYxffd83Sx#4TEh2bs4PBAF9fChgNSufwNPRgWtfgTmHJn2PUPTn7tW

Events:

EVENT_JSON:{"standard":"x-nearedu-cert","version":"1.0.0","event":"cert_issue","data":"{\"Issue\":{\"recipient_id\":\"hatchet.testnet\",\"token_id\":\"103216412112497cb6c193152a27c49a\",\"memo\":null}}"}
EVENT_JSON:{"standard":"nep171","version":"1.0.0","event":"nft_mint","data":[{"owner_id":"hatchet.testnet","token_ids":["103216412112497cb6c193152a27c49a"]}]}

Testnet wallet screenshot:
2022-04-12-09-04-53

@ryancwalsh
Copy link
Contributor Author

@encody Ok. Since I still don't see it at https://wallet.testnet.near.org/profile/hatchet.testnet I'm guessing I never was supposed to be able to see it there. It seems like only you (when logged in) can see it. That kind of makes sense to me, because I don't think our protocol should allow people to forcefully spam other people's wallets.

But I remember conversations about this. I wonder if Wallet "fixed" this (ie. maybe Wallet in the past displayed all of an account's NFTs by default but no longer does).

If yes, I wonder if there is a way for an owner of a wallet to manually choose which NFTs to display publicly. But that's just a side question and not a blocker here.

I will also try minting one to my own account now and see whether it works. Steps:

  1. Pulled fresh develop branch
  2. ./build.sh
  3. Temporarily change init_args.json to have "owner_id": "ryancwalsh.testnet",
  4. ./dev_deploy.sh --force
  5. Edit ./data-contract/sample_mint.json to have unique ID and "original_recipient_id": "ryancwalsh.testnet"
  6. NEAR_ENV=testnet near call dev-1649780593510-28371141287670 nft_mint "$(<./sample_mint.json)" --account-id ryancwalsh.testnet --deposit 0.2 --gas 300000000000000
  7. Confirm that the nft_mint event appears in the transaction (it does at https://explorer.testnet.near.org/transactions/7TVa5i8z45HiydyuH8HgeiukjCCUVqGNFZ3EB12Cv7os)
  8. Confirm that https://wallet.testnet.near.org/?tab=collectibles (logged in as ryancwalsh.testnet) shows the NFT. (It does, and it links to https://wallet.testnet.near.org/nft-detail/dev-1649780593510-28371141287670/e03216412112497cb6c193152a27c49a)

Why do you think the images are broken in these 2 views?
image

image

What does the "icon" field at https://explorer.testnet.near.org/transactions/3NM7EjhiJzZReKBZoCsnAMVnydkHiPfqo46kHNb46VDi do? I tried to set the N logo as the icon. And we do see an N logo (in the first screenshot, to the left of NEAR University Certificate). So does that mean the icon is working? But both screenshots have broken images too. Where are those supposed to have been defined?

@ryancwalsh
Copy link
Contributor Author

ryancwalsh commented Apr 12, 2022

@encody

  1. Also, why do you think NFTs weren't appearing before (and now they do, even though we didn't change the contract)? Just a temporary problem with the indexer?

  2. Also, my latest minted NFT is not appearing at http://localhost:3000/account/ryancwalsh.testnet! (yet?). I wonder if I need to wait an hour or something? NEAR_ENV=testnet near view dev-1649780593510-28371141287670 nft_tokens_for_owner '{"account_id": "ryancwalsh.testnet"}' successfully shows it.

UPDATE about #2: Oops, never mind. I'd forgotten to update NEXT_PUBLIC_CERTIFICATE_CONTRACT_NAME in .env.local and restart my local server. I'm still curious about the broken images mentioned in my previous comment and my #1 question above.

@ryancwalsh
Copy link
Contributor Author

@encody
3. Assuming NEAR University eventually has its own logo, will the "icon" field of this contract be editable / upgradable? If yes, will it affect all certs or only certs minted after that point?

@encody
Copy link
Contributor

encody commented Apr 12, 2022

@ryancwalsh The broken images appear to be where the wallet would put additional metadata specified in the reference field:

  icon: string|null, // Data URL
  base_uri: string|null, // Centralized gateway known to have reliable access to decentralized storage assets referenced by `reference` or `media` URLs
  reference: string|null, // URL to a JSON file with more info
  reference_hash: string|null, // Base64-encoded sha256 hash of JSON from reference field. Required if `reference` is included.

Here is an example of what that metadata might look like:

nft_metadata call:

{
  "spec": "nft-1.0.0",
  "name": "deadmau5",
  "symbol": "dm5",
  "icon": "data:image/x-icon;base64,AAABAAEAEBAQAAEABAAoAQAAFgAAACgAAAAQAAAAIAAAAAEABAAAAAAAgAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAJCT/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEQAAEQAAAAEAEAEAEAAAAQAQAQAQAAAAEREREQAAAAAAEAAAAAAAAAAQAAAAAAAAABEQAAEQAAAAEAEAEAEAAAAQAQAQAQAAAAEREREQAAAAAAAAEAAAAAAAAAAQAAAAAAAAERAAAAAAAAEAEAAAAAAAAQAQAAAAAAAAEQAADnnwAA228AANtvAADgHwAA+/8AAPv/AAD48wAA+20AAPttAAD8AwAA/+8AAP/vAAD/jwAA/28AAP9vAAD/nwAA",
  "base_uri": "https://arweave.net",
  "reference": null,
  "reference_hash": null
}

nft_token call:

{
  "token_id": "241272",
  "owner_id": "jeph.near",
  "approved_account_ids": {},
  "metadata": {
    "title": null,
    "description": null,
    "media": null,
    "media_hash": null,
    "copies": 100,
    "issued_at": null,
    "expires_at": null,
    "starts_at": null,
    "updated_at": null,
    "extra": "music",
    "reference": "nb0-oBR379DzoFYeYv-LesjVNmrVlDs5IqQ8hfDfnMU",
    "reference_hash": null
  },
  "royalty": {
    "split_between": {
      "portugaltheman.near": {
        "numerator": 3750
      },
      "deadmau5.near": {
        "numerator": 3750
      },
      "woodencyclops.near": {
        "numerator": 500
      },
      "smearballs.near": {
        "numerator": 2000
      }
    },
    "percentage": {
      "numerator": 1000
    }
  },
  "split_owners": null,
  "minter": "mau5trap.near",
  "loan": null,
  "composeable_stats": {
    "local_depth": 0,
    "cross_contract_children": 0
  },
  "origin_key": null
}

The base_uri plus the reference points to https://arweave.net/nb0-oBR379DzoFYeYv-LesjVNmrVlDs5IqQ8hfDfnMU:

{
  "category": "music",
  "description": "Single by deadmau5 x Portugal. The Man\n@deadmau5 @portugaltheman\n \nArtwork by Smearballs x Wooden Cyclops\n@smearballs @woodencyclops \n \n1 million units minted on the NEAR protocol, with additional utility and integrations in the metaverse.",
  "copies": 1,
  "media_hash": "iZ_zHjbOs8rqAQDHYVE2moonW80ydYwkx7VBNzHdS5A",
  "lock": null,
  "visibility": "safe",
  "youtube_url": null,
  "animation_url": "https://arweave.net/uirST9TAzYywSKj6nuvV9Ki_4LBxuzBko9xH8OAalnI",
  "animation_hash": "uirST9TAzYywSKj6nuvV9Ki_4LBxuzBko9xH8OAalnI",
  "document": null,
  "document_hash": null,
  "royalty": {
    "smearballs.near": 2000,
    "woodencyclops.near": 500,
    "portugaltheman.near": 3750,
    "deadmau5.near": 3750
  },
  "royalty_perc": 0.1,
  "split_revenue": {
    "smearballs.near": 2000,
    "woodencyclops.near": 500,
    "portugaltheman.near": 3750,
    "deadmau5.near": 3750
  },
  "tags": [],
  "media": "https://arweave.net/iZ_zHjbOs8rqAQDHYVE2moonW80ydYwkx7VBNzHdS5A",
  "extra": [
    {
      "trait_type": "website",
      "value": "https://deadmau5.com"
    }
  ],
  "title": "this is fine",
  "store": "deadmau5.mintbase1.near",
  "external_url": "https://deadmau5.com",
  "type": "NEP171"
}

of which media field:

https://rgompdntvtzlvacaghmfitnguke5n42mtvrqsmpnkbg4y52s4q.arweave.net/iZ_zHjbOs8rqAQDHYVE2moonW80ydYwkx7VBNzHdS5A

However, that's just the first NFT I bothered to look up. My guess is that we can just use the built-in media field and we don't have to go through this reference nonsense.

@ryancwalsh
Copy link
Contributor Author

@encody Ok, I can try using the media field tomorrow.

Here were my other questions from above:

1. Why do you think NFTs weren't appearing before (and now they do, even though we didn't change the contract)? (Do you think it was just a temporary problem with the indexer?)

3. Assuming NEAR University eventually has its own logo, will the "icon" field of this contract be editable / upgradable? If yes, will it affect all certs or only certs minted after that point?

@ryancwalsh
Copy link
Contributor Author

@encody The media field worked. See 81fc2f4

https://wallet.testnet.near.org/nft-detail/dev-1649780593510-28371141287670/f03216412112497cb6c193152a27c49a shows:

image

And https://wallet.testnet.near.org/?tab=collectibles shows the old one still as broken but the new one with the N logo:

image

@encody
Copy link
Contributor

encody commented Apr 12, 2022

@ryancwalsh

1. Why do you think NFTs weren't appearing before (and now they do, even though we didn't change the contract)? (Do you think it was just a temporary problem with the indexer?)

I think that the contract was being built using an old version of the near-contract-standards crate.

3. Assuming NEAR University eventually has its own logo, will the "icon" field of this contract be editable / upgradable? If yes, will it affect all certs or only certs minted after that point?

Currently the contract implementation does not provide any way to alter the metadata after deployment. I can add that functionality (or maybe only the option to modify certain fields, such as icon) without very much trouble.

@ryancwalsh
Copy link
Contributor Author

@encody

I can add that functionality (or maybe only the option to modify certain fields) without very much trouble.

Great. Can you please do that? Because we're nearly ready to deploy to mainnet and get this going, finally. E.g. maybe @petarvujovic98 and I could be ready as soon as tomorrow or the next day.

I think we probably want to leave everything (rather than just icon) editable. Although knowing how to do either option would be cool.

I think it will be a very interesting learning experience for me (and maybe others) to see how you do it. Thanks.

encody added a commit that referenced this issue Apr 12, 2022
@encody
Copy link
Contributor

encody commented Apr 12, 2022

@ryancwalsh
Copy link
Contributor Author

@encody

Very cool. Thanks!

  1. Is there any other step we'd need to take to make the entire contract upgradable (rather than just the metadata), such as if we wanted (for some reason) to edit/add/remove functions?

  2. Given that we already deployed a previous version of the contract to https://explorer.near.org/accounts/certificates.unv.near, what are the steps for overwriting that contract with the latest build? Related to question #1, are we only able to do this currently because we are willing to delete / abandon any existing user experience / data (since Rasha is currently the only person who received a cert on mainnet)? If we'd given certs to lots of people, would we be out of luck here?

@encody
Copy link
Contributor

encody commented Apr 12, 2022

@ryancwalsh

  1. Is there any other step we'd need to take to make the entire contract upgradable (rather than just the metadata), such as if we wanted (for some reason) to edit/add/remove functions?

Possibly. This could be accomplished in a few ways:

  1. Maintaining a full-access key to the account. This is the simplest, most straightforward, and also most trusted solution.

  2. Adding an "upgrade" function to the smart contract that redeploys code to the current account, overwriting the existing code. I haven't tested this code, but it might look something like this:

    pub fn clear(&mut self, keys: Vec<Base64VecU8>) {
        // Force owner
        self.assert_owner();
        // Force verification
        assert_one_yocto();
    
        for key in keys.iter() {
            env::storage_remove(&key.0);
        }
    }
    
    pub fn upgrade(&mut self, code: Vec<u8>) -> Promise {
        // Force owner
        self.assert_owner();
        // Force verification
        assert_one_yocto();
    
        // Redeploy contract
        Promise::new(env::current_account_id()).deploy_contract(code)
    }

    Particularly the Redeploy contract line might require that the contract (temporarily) add the public key of the current transaction signer (the contract owner) to the current account. This is probably a security issue due to the asynchronous nature of promise resolution. Not sure if this would work.

  3. Deploying one contract ("certificates.unv.near") as a version manager / false-proxy contract that performs two functions:

    1. Deploys contracts to sub-accounts ("v1.certificates.unv.near", "v2.certificates.unv.near", etc.)
    2. Maintains a pointer to the most recently-deployed sub-account

    Then our application would just have to know to ask the proxy contract for the account ID of the real contract.

  1. Given that we already deployed a previous version of the contract to https://explorer.near.org/accounts/certificates.unv.near, what are the steps for overwriting that contract with the latest build? Related to question #1, are we only able to do this currently because we are willing to delete / abandon any existing user experience / data (since Rasha is currently the only person who received a cert on mainnet)? If we'd given certs to lots of people, would we be out of luck here?

Assuming you still have a full-access key on that account, simply running another deploy contract transaction will overwrite the contract. State will be preserved, so Rasha will still have her certificate, assuming that the storage keys are unaltered.

@ryancwalsh
Copy link
Contributor Author

@encody Cool, thank you so much! Yeah I don't think there will be a reason for us ever to delete the private key to this account, so it sounds like we're set. And the other options were good to hear about too for other projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hard High Priority
Projects
None yet
Development

No branches or pull requests

2 participants