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

Equipping into an NFT with 2 resources of same base #18

Open
Swader opened this issue Mar 31, 2022 · 15 comments
Open

Equipping into an NFT with 2 resources of same base #18

Swader opened this issue Mar 31, 2022 · 15 comments

Comments

@Swader
Copy link
Contributor

Swader commented Mar 31, 2022

Screenshot 2022-03-31 at 14 47 51

We should perhaps modify the Equip logic such that it pays attention to which resource is is equipping into as well.

@steven2308
Copy link
Collaborator

I will assume a "base instance" in this context is a resource pointing to a base. (There are no such things as base instances, bases are just for reference).
In the current implementation this is not possible and I don't think it should be. We allow one resource to point to a single base and a single slot. This makes it easier on code and also avoids unexpected behavior, like a weapon resource meant for the left hand being equipped on the right hand or showing 2 weapons in a single hand.

We keep track from the parent context (say a bird) that only one NFT is equipped into each token/base/slot combination. We also keep track from the child context (say a hat) that it is only equipped into one NFT at a time (this is important for scarcity).

Just FYI, we also check on the base, that the child which wants to be equipped is meant to be in that slot for the base, this is to avoid bad use of the base, only the issuer can declare what collections can be used for the base (or if a slot part is equippable by any collection).

Acout also proposed an interesting example of a soldier loosing a hand after a war, so the hand shouldn't be able to equip weapons anymore. In this case, the solution would be to send a new resource to the token (possibly replacing the original one). This new resource would use a new Base, which does not have a slot for the lost hand.

@Swader what are your thoughts on this?

@Ayuilos
Copy link
Contributor

Ayuilos commented Aug 4, 2022

In fact, what I want to say will also not cause the situation mentioned in steven's bold text. I think there is a concept of Base instance, because when we use Base, we do not directly quote but cherry-pick, then every resource that uses Base is an instance of Base.
Therefore, it is reasonable to have multiple instances referencing the same Base in an NFT.

For more context please let steven, me or cicada invite you Mr. Bruno to our discussion group.

I think there's a need to reach a consensus.

@Ayuilos
Copy link
Contributor

Ayuilos commented Aug 4, 2022

I will assume a "base instance" in this context is a resource pointing to a base. (There are no such things as base instances, bases are just for reference). In the current implementation this is not possible and I don't think it should be. We allow one resource to point to a single base and a single slot. This makes it easier on code and also avoids unexpected behavior, like a weapon resource meant for the left hand being equipped on the right hand or showing 2 weapons in a single hand.

We keep track from the parent context (say a bird) that only one NFT is equipped into each token/base/slot combination. We also keep track from the child context (say a hat) that it is only equipped into one NFT at a time (this is important for scarcity).

Just FYI, we also check on the base, that the child which wants to be equipped is meant to be in that slot for the base, this is to avoid bad use of the base, only the issuer can declare what collections can be used for the base (or if a slot part is equippable by any collection).

Acout also proposed an interesting example of a soldier loosing a hand after a war, so the hand shouldn't be able to equip weapons anymore. In this case, the solution would be to send a new resource to the token (possibly replacing the original one). This new resource would use a new Base, which does not have a slot for the lost hand.

@Swader what are your thoughts on this?

I think the core problem is here:

In this case, the solution would be to send a new resource to the token (possibly replacing the original one). This new resource would use a new Base, which does not have a slot for the lost hand.

We don't need this slot by not cherry-picking this slot is enough, we still use the same Base, maybe there's no need to add a new Base.

@steven2308
Copy link
Collaborator

steven2308 commented Aug 4, 2022

Slots parts cannot be cherry picked. When a resource uses a base (and it can only use one), it can receive NFTs in all the slots that the base defines.
You can only cherry pick fixed parts for a resource. For instance, if body parts are defined in the base, your new resource could simply not pick a fixed part for the arm, but unless it uses a different base you would still be able to equip a weapon into a missing arm. So you either use a different base without the slot, or wrap the equipping logic to control that on your contract.

This functionality of cherry picking slot parts is not in the spec AFAIR, it would make everything much more complex, and there's a simple alternative of just using a new resource with a different base.

@Ayuilos
Copy link
Contributor

Ayuilos commented Aug 4, 2022

I think the biggest benefit of "cherry-pick" is that you can define a Base at once that is perfect enough that it can be effectively reused in large numbers to build a Base ecosystem.

We should not compromise in order to reduce the complexity of the contract code, and I think that even if one NFT can have multiple instances of the same Base, it will not bring too much new logic, because there is no problem from the perspective of structure design:

struct Equipment {
        uint64 resourceId;
        uint64 childResourceId;
        uint childTokenId;
        address childEquippableAddress;
}

, only the _equipments mapping:
mapping(uint => mapping(address => mapping(uint64 => Equipment))) private _equipments;
has to redesign a little.

I can submit a PR to prove it has no breaking change to any existing logic.

@steven2308
Copy link
Collaborator

steven2308 commented Apr 26, 2023

@Ayuilos I'm revistting this after some use case I saw where using the asset ID instead of the catalog for the equipements mapping, would be a huge issue.
However I see that we already use the catalog and that you were suggesting the same. Is there any issue left? This is what we're using

/// Mapping of token ID to catalog address to slot part ID to equipment information. Used to compose an NFT.
mapping(uint256 => mapping(address => mapping(uint64 => Equipment)))
    private _equipments;

@Ayuilos
Copy link
Contributor

Ayuilos commented Apr 26, 2023

Any example for pointing that using the asset ID for equipments mapping would be a huge issue?

@steven2308
Copy link
Collaborator

Say I have a bag NFT which has 10 slots. I have a few NFTs equipped into those.
I upgrade my bag, so I receive a new asset which has now 12 slots. This is a new asset so it has a different asset Id. All the equipments are invalid now. Alternative would be to ask user to unequip everything first which is very bad UX.

However, if it's linked through the catalog, there's no issue at all. I'm sure this same scenario applies to several cases where you upgrade your asset to have more slots.

@Ayuilos
Copy link
Contributor

Ayuilos commented Apr 26, 2023

I think this is a trade-off in terms of choice.
If I use asset ID for equipments mapping, it will perfectly fit ERC5773 philosophy.
For the situation you mentioned above, I think we don't need to remove equipments on old asset, just needing to replay the equip action on new asset.
What's more, I think asset replacement is not something someone really want. Because someone would like to check his(her) nft's growth record from evolution 1 -> evolution 3, the asset replacement will let only evolution 3 left.

@steven2308
Copy link
Collaborator

just needing to replay the equip action on new asset.

This is not a "just", it's expensive an bad UX.
For historical data, we recommend indexers always.

@steven2308
Copy link
Collaborator

So just to be clear, you're suggestion is to map through asset id, right?

@Ayuilos
Copy link
Contributor

Ayuilos commented Apr 26, 2023

Yes because I think only in this way is respecting ERC-5773 philosophy.
The situation you mentioned above is adding custom logic to auto replace equipments, rather than breaking ERC-5773 definition.

@steven2308
Copy link
Collaborator

I don't understand why you consider this would not be respecting 5773, could you elaborate? What are we breaking from it's definition?

@Ayuilos
Copy link
Contributor

Ayuilos commented Apr 26, 2023

OK

We know that the definition of the ERC-5773 is that the NFT produces different outputs based on its context. Each asset, in fact, reflects the output of the NFT in some context. This is actually a philosophical description of "There are a thousand Hamlets in a thousand people's eyes." What we see is the projection of the NFT into our perspective, and the NFT itself is an abstraction.

My understanding of the equipment properties, then, is that the equipment properties allow NFT A and NFT B to enter the same context and produce corresponding chemical reaction projections based on the ownership relationship brought about by the ERC-6059, so the ERC-6220 is a special representation of the ERC-5773.

If you agree with me above, then the equiping behavior should occur on the assets, because each asset should correspond to a separate context. This is the foundation of ERC-5773, and now ERC-6220 says that all assets referencing the same Catalog should correspond to the same context. This breaks the most fundamental definition of ERC-5773.

@Ayuilos
Copy link
Contributor

Ayuilos commented Apr 26, 2023

Let's take an example where we add two ordinary assets but the metadata of both assets may point to the same URI for different reasons, can you assume that they are the same asset just because they have the same metadata?

If a light shines on an object, the light shines on the left or the light shines on the right, we may see the same projection on the wall, but the shines action is different.

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

3 participants