search

violetprotocol / ERC1238-token

Implementation of EIP-1238 for non-transferable tokens (fungible & non-fungible).
https://erc1238.notion.site/
16 stars 3 forks source link

Add tests for ERC1238 and ERC1238URIStorage #3

Closed ra-phael closed 2 years ago

ra-phael commented 2 years ago

Add some tests for ERC1238 and the extension to manage token URIs.

Apologies for the long PR, I had to add hardhat tooling etc..

0xpApaSmURf commented 2 years ago

Great stuff! Looks super clean, well done :D

I see that we are using the original mapping structure for storing balances. In this case, maybe it would be useful to describe how an Owner or otherwise manager of the contract would encode an NFT vs FT in practice? The suggestion you mentioned previously could be one of those ways but since it's up to the operator/owner during runtime to decide, it would be best to document it and give an example or 2.

I think the suggestion to migrate it to the TokenCollection struct makes this separation much easier to distinguish and understand. Is that something you'd decided not to pursue or is that for another version?

ra-phael commented 2 years ago

Thank you @0xpApaSmURf!

I totally agree with you, it's sadly lacking in documentation at the moment. That will be added ASAP, along with improving the README in another PR. If by encoding you mean the bit packing stuff, I just pushed a branch, it's: https://github.com/violetprotocol/ERC1238-token/tree/semi-fungible

And same for the struct, it's here: https://github.com/violetprotocol/ERC1238-token/tree/variant-with-struct But both are still WIP

0xpApaSmURf commented 2 years ago

Eventually though, one of the two approaches will prevail. Isn't it duplication (or redundant work) to implement both and then to supersede one later on?

I think it would be better to take a decision on approach as we were discussing in a previous thread and then just focus on that implementation to avoid wasting time.

ra-phael commented 2 years ago

The idea was to do some "exploratory work" to have a better understanding of what implementations would look like and try to gather feedback before taking a decision but let's discuss it. However if that's okay with you I need to merge this branch as it contains all the tooling used by other branches.

0xpApaSmURf commented 2 years ago

All good!