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

Add ability to get NFTs owned by a user to Marmalade #24

Open
kitty-kad opened this issue Jul 5, 2022 · 10 comments
Open

Add ability to get NFTs owned by a user to Marmalade #24

kitty-kad opened this issue Jul 5, 2022 · 10 comments

Comments

@kitty-kad
Copy link

kitty-kad commented Jul 5, 2022

Problem:
There is no standard and easy way to show all the NFTs owned by a user when looking at NFTs in the current marmalade ledger.
This makes it difficult to create nice user experiences which allow users to easily select NFTs they own.

Proposal
Add a new function get-nfts-for-owner to the token-policy interface which will return all the NFTs for a given user

Current alternatives
Look up events related to ownership transfer. However this

  • Becomes tricky as it requires everyone who wants to make a new UI to track all the NFT transfer requests. This increases the barrier to entry and will inevitably reduce the rate of innovation on the ecosystem.
  • Is becoming more challenging due to the fragmentation of ledgers. Only one project is "live" and they're not using the default Marmalde ledger. This is possible to work-around but adds to the complexity of things.
@kitty-kad
Copy link
Author

In ERC 721 this seems similar to the optional tokenOfOwnerByIndex method

@emilypi
Copy link
Member

emilypi commented Jul 6, 2022

The question for me is not whether there is utility in this function, but whether it's the responsibility of the interface, the specific NFT contract implementation, or the service providing the NFTs. Consider what happens in each of the 3 scenarios:

  1. get-nfts-for-owner is a function of the interface: This requires the fetched NFT data be stored on chain by everyone forever, as a requirement for having an NFT contract. The only way to really fetch this data is with an extremely costly (by design) SELECT statement for the associated contract ledgers. The gas cost for this may be nontrivial, and shunting this to either the end user or the service is suboptimal.
  2. get-nfts-for-owner is a function of the contract implementation: This case offers the flexibility of not implementing this in other NFT contracts, which allows others to opt in to the expensive call, at the cost of gas. They can also customize and optimize solutions per their own spec.
  3. get-nfts-for-owner does not exist in the contract or implementation: Service providers have a fallback mechanism that already exists that can provide all the necessary data for aggregating the tokens associated with minted NFTs for a given user by means of watching for BUY events produced by the BUY capability in the ledger. This allows for custom data storage solutions that don't require the end user or the provider to suffer a potentially volatile retrieval cost (modulo fiat storage overhead).

To me, 2 and 3 are the most viable options, with the 3rd being the "optimal" solution in my mind - it already exists, and data storage is both opt-in, and potentially more cost-effective for both party and counterparty. However, depending on how people feel about this, I'm open to 2.

Thoughts?

@amilano
Copy link

amilano commented Jul 6, 2022

@emilypi regarding 3, I've found that RECONCILE was easier to leverage for the purpose of reconstructing the contents of an account's "wallet" since you can use the current attribute either with sender or receiver. Using chainweb-data and querying directly the PG database is really fast so far.

Regarding the SELECT statement, why gas is an issue when executing a Pact.fetch.local? if gas is not an issue, could the function using SELECT be restricted to be used only for local calls, is that even a restriction that could be set?

@kitty-kad
Copy link
Author

kitty-kad commented Jul 6, 2022

@emilypi

  1. .... The gas cost for this may be nontrivial, and shunting this to either the end user or the service is suboptimal.

Two points on that

  • This is an only an issue for large collections
  • A large part of this proposal it to allow user facing Dapps to know which NFTs a user has. Since read-only calls to chainweb nodes are free, gas isn't an issue (as mentioned above)
  1. ... They can also customize and optimize solutions per their own spec.

The issue here is that there will be multiple different specs which will lead to difficulties making UIs to handle all kinds of NFTs. It's not very scalable from a web developer side.
As for implementation details - that can be left to each person to implement their own, the method can just be an interface that needs to be filled out on the token-policy

  1. ... Service providers have a fallback mechanism that already exists that can provide all the necessary data for aggregating the tokens associated with minted NFTs for a given user by means of watching for BUY events produced by the BUY capability in the ledger.

This is precisely the issue I'm proposing to avoid. The two issues with this are

  1. The barrier to entry is high. Anyone who wants to implement any NFT selection functionality on the UI will be required to monitor all ledgers and also write / deploy a system for keeping track of the latest state of the world.
  2. Listening to BUY, SELL or RECONCILE events is not reliable. We already have 2 different ledgers. It's extra challenging (not impossible though) to keep up to date with all the ledgers being deployed.
    Listening for generic .*.RECONCILE events doesn't work either because there is no guarantee that other (non-marmalade) contracts won't have a RECONCILE event. Again, there are work arounds but it adds a high barrier to entry for people make new Dapps

@emilypi
Copy link
Member

emilypi commented Jul 7, 2022

@emilypi regarding 3, I've found that RECONCILE was easier to leverage for the purpose of reconstructing the contents of an account's "wallet" since you can use the current attribute either with sender or receiver. Using chainweb-data and querying directly the PG database is really fast so far.

Nice that's a good datapoint for people building on the thing. Glad it's a quick experience!

Regarding the SELECT statement, why gas is an issue when executing a Pact.fetch.local?

Gas is not an issue when dry-running, but dry-running computation does come at a cost to the nodes. If you or your organization have a node set up, then that's great - spam all you like. However, I imagine node operators would not be too stoked on having people hit them with arbitrarily complex computations in general.

The question is whether or not we want expensive computational data to be propagated throughout the network unless it's absolutely necessary. So, my bias here is towards the health of the network: market data and associated computations that are not absolutely necessary for the contract to function should remain off chain. Remember - anything that happens on chain is forever. Do we really need it when events suffice, even if more complicated to monitor? If so, we can provide help for off-chain market data computation in the form of more complex additions to the API. Currently, a GraphQL endpoint is in the works, which would make data aggregation of this kind easier for services, however, this will not be available for a few months. But because we can do this off-chain, I'm leaning towards that as a solution.

if gas is not an issue, could the function using SELECT be restricted to be used only for local calls, is that even a restriction that could be set?

This would effectively let the service API bleed into the language design. Pact needs to remain agnostic of such things as a language. Chainweb is not the only one using Pact, and node operators are free to turn individual end points on or off as they see fit.

Two points on that

  • This is an only an issue for large collections
  • A large part of this proposal it to allow user facing Dapps to know which NFTs a user has. Since read-only calls to chainweb nodes are free, gas isn't an issue (as mentioned above)

Regarding the first point: yes, point taken. However, we take a defensive tack towards this kind of thing. If some service decides to mint a billion NFTs and sell them extremely cheaply, that's a billion records that get to be stored on chain in perpetuity and constantly read-accessed on chain if anyone decides to not use the /local endpoints. Currently, every function in a smart contract is public. Since there is no notion of a private API, everything in the contract is potentially callable both on-chain, and as a dry-run.

The issue here is that there will be multiple different specs which will lead to difficulties making UIs to handle all kinds of NFTs. It's not very scalable from a web developer side.
As for implementation details - that can be left to each person to implement their own, the method can just be an interface that needs to be filled out on the token-policy

I agree - 2 is suboptimal for this reason.

The barrier to entry is high. Anyone who wants to implement any NFT selection functionality on the UI will be required to monitor all ledgers and also write / deploy a system for keeping track of the latest state of the world.

Yes. Ideally, if you're offering a service to people, you'd be running your own node and chainweb-data services for aggregating your service-specific market data. The solution in my mind here is not "throw it on chain", but rather, we need to provide an easier means of aggregating that data. Not everyone will need this data is the fundamental problem.

I think my main balk here is the fact that we're asking for get-nfts-by-owner, which is fundamentally different from exposing a function that allows one to do the ERC721Enumerable thing of tokenOfOwnerByIndex. The former asks that we get all indices for a given owner, but the latter only allows for individual queries via pairs of owner and index - presumably to avoid the problem of querying for unnecessary data, which the former suffers from.

Listening to BUY, SELL or RECONCILE events is not reliable. We already have 2 different ledgers. It's extra challenging (not impossible though) to keep up to date with all the ledgers being deployed. Listening for generic .*.RECONCILE events doesn't work either because there is no guarantee that other (non-marmalade) contracts won't have a RECONCILE event. Again, there are work arounds but it adds a high barrier to entry for people make new Dapps

But if you're listening for kiddykads.marmalade-token.*, you'll have everything you need in terms of events, and then you can filter on *. I'm confused as to why this in particular does not work. Certainly you don't want to be aggregating all reconcile events, but even if you did, the namespace and module name provide a globally unique identifying prefix you can use as a key. If this needs to be made easier so that we lower the barrier to entry, I'm all for that. But it's a separate proposal.

@emilypi
Copy link
Member

emilypi commented Jul 7, 2022

But it's a separate proposal.

And ftr, I'm happy to help work on that so we get you folks everything you need in terms of tooling for chainweb-data. In-prod user and developer feedback is the most important kind.

@kitty-kad
Copy link
Author

Thanks for the response ! Appreciate you going through the pints

In regards to

But if you're listening for kiddykads.marmalade-token.*, you'll have everything you need in terms of events, and then you can filter on *.

The issue isn't for kitty kads, but for making generic services that let people select and interact with their nfts on different smart contracts .

Examples are

  • wallet to see your nfts
  • marketplace to select your nfts to list
  • staking service for generic nfts where you select an nft to stake

This is why having a consistent way to quickly and easily get data about nfts is useful

et-nfts-by-owner, which is fundamentally different from exposing a function that allows one to do the ERC721Enumerable thing of tokenOfOwnerByIndex

I think the methods are roughly equivalent and the difference is language implementation to an extent .

IDs in pact tables are similar to indices in solidity.
Having a method to expose ids by owner would also work - though combining the methods (get ids and fetch data for id) is nice .

But practically having a method to get all ids for a user would also work

I imagine node operators would not be too stoked on having people hit them with arbitrarily complex computations in general.

Node operators could also charge users for accessing their nodes for read only / local reads . I think on ethereum some nodes already do this .
The issue with relying on events history is that event data is a lot more niche so there'll be much less services offering things like nft data.
In theory people could spin up their own nodes and index in their own db. In practice there'll be services offering this solution as a service and due to its nicheness will lead to centralisation where a few services power most front ends , which will be an issue for decentralisation.

associated computations that are not absolutely necessary for the contract to function should remain off chain

Side question about this philosophy - is marmalade storing the nft metadata on chain going against this principle ? Since we just need to store the hash of the metadata on chain, things like the features of nfts etc can all be stored off chain and loaded via a uri ?

@jmininger
Copy link

I don't think we'd be opposed to an extension interface (much like interface ERC721Enumerable) that policies can opt-in in order to use. The implementation could use the enforce-* suite of functions to build up an ownership index inside of the token-policy, and the extension would provide the functions used to access the index.

@kitty-kad
Copy link
Author

I've raised another proposal which handles this and a few other issues related to making Dapps easier.
#25

At the moment the proposal doesn't use methods like tokenOfOwnerByIndex but could be adjusted to do so.

I can close off this issue or keep it open as it is possible to decouple the two

@sirlensalot
Copy link
Contributor

Token policies (token-policy-v1) are the right approach and works with current Marmalade standard.

A token policy can be shared across collection entries really easily.

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

No branches or pull requests

5 participants