Comments/questions on SwappableNFT

[radrow]

These are things that I caught while reviewing SwappableNFT. Some of those look like bugs to me, some could just make use of Sophia better, and some I just don't get the intention of, but I decided that it's better to point them out, than let a potential bug go.

Some of the comments apply to other types of NFT as well. I will try not to duplicate them in there while reviewing the rest.

So here you are:

Line 79

This seems unnecessary, you can use Contract.creator (it would make sense it
this field was ever overwritten, but it's not)

Line 98

Operation of getting and incrementing counter could be a separate function.

Line 95

A good pattern is to add more meaning to types, so it's harder to make a bug.
Here, token_id is not just an arbitrary number used in computation, but a
token identifier. Thus, it would be poor to mistake it with balance for example
-- typechecker won't help you here!

To make it more defensive, I recommend creating a record type

record token_id = {value : int}

This has zero runtime (gas) overhead, but will make it much harder to make a bug
upon it.

Line 103

Is it fine for different tokens to have the same metadata?

Line 54

I have a hunch that you wanted Map(map(string, string))... Also, why not

datatype metadata = URL(string) | IPFD(string) | OBJECT_ID(string) | Map(map(string, string))

Is it wrong to have different kinds of NFTs in a single contract? The contract
needs to handle all cases anyway. At least at this level I don't see much
sense for keeping them forcefully uniform.

Line 114

The None case should be handled.

Line 121

account is unused. Did you mean to lookup by it instead of the caller?

Line 153

You can use String.concat to concat two strings

Line 151

It's still good to consider other cases and throw a message that would describe
what happened. Even though it's impossible, we all know that impossible happens.
And if you are really certain, you can write let String(s) = v to avoid
nesting.

Line 67

Why can you approve only a single address?

Line 68

Didn't you want to use set from Set.aes maybe?

Line 182

Why do we need to specify from as it comes from the state?

Line 217

Naming inconsistency: functions owner and get_approved

Line 103

You don't check for consistency with metadata_type, but the rest of the code makes assumptions on that

Line 82

What is balance really used for? As I look it stores the amount of tokens one
has, which can be transfered to some map that is never read

[marc0olo]

Line 79

This seems unnecessary, you can use Contract.creator (it would make sense it this field was ever overwritten, but it's not)

is Call.caller more expensive? or just a convenience thing?

Line 98

Operation of getting and incrementing counter could be a separate function.

@arjanvaneersel would you mind adjusting the code? :)

Line 95

A good pattern is to add more meaning to types, so it's harder to make a bug. Here, token_id is not just an arbitrary number used in computation, but a token identifier. Thus, it would be poor to mistake it with balance for example -- typechecker won't help you here!

To make it more defensive, I recommend creating a record type

record token_id = {value : int}

This has zero runtime (gas) overhead, but will make it much harder to make a bug upon it.

generally agree. but not sure how to deal with it. we'd also have to adjust the standard definition again I guess.

Line 103

Is it fine for different tokens to have the same metadata?

actually not sure if we should prevent that. but it could make sense to avoid duplicate mints. especially as an NFT in general should be considered unique (at least in a given context). what do you think @arjanvaneersel @thepiwo?

Line 54

I have a hunch that you wanted Map(map(string, string))... Also, why not

datatype metadata = URL(string) | IPFD(string) | OBJECT_ID(string) | Map(map(string, string))

Is it wrong to have different kinds of NFTs in a single contract? The contract needs to handle all cases anyway. At least at this level I don't see much sense for keeping them forcefully uniform.

first
why would we want Map(map(string, string))? can you elaborate on that one?

second
good question. I was thinking about this lately again. we had some discussion around this back then. I suggest you to read through the initial discussion: aeternity/AEXs#141

back then we came to the conclusion that it's very unlikely that a specific NFT contract (or project) mixes up different type of metadata.

the way we have it right now makes it very easy for consumer applications like marketplaces to show the NFTs in the right way.

if there are good reasons we could still change the behavior as long as not widely adopted.

also we still lack of definitions how to deal with metadata content. IPFS for example could be an image or video, but it could also represent a JSON file (which I prefer) that contains multiple attributes, including the IPFS hash of an image/video.

there is many possibilities here that we provide. maybe it's even better to store all the attributes in the key value map where you wouldn't have to look up attributes via IPFS and have them directly written in the state of the contract.

Line 114

The None case should be handled.

we should abort in this case, right? I also saw the open todo in the comments of the implementation. what's your opinion here @arjanvaneersel @thepiwo ? I think we should also remove from owners and metadata as well. do we have an efficient way to do it or could this become a problem if somebody owns a lot NFTs?

Line 121

account is unused. Did you mean to lookup by it instead of the caller?

I guess @arjanvaneersel wanted to provide the option to provide an account and use Call.caller if no account is provided. but then it'd be better to use option(address) as param. this would require a change in the standard definition.

IMO we can just omit Call.caller and use account instead.

Other opinions? @arjanvaneersel @thepiwo

Line 153

You can use String.concat to concat two strings

@arjanvaneersel would you mind adjusting the code? :)

Line 151

It's still good to consider other cases and throw a message that would describe what happened. Even though it's impossible, we all know that impossible happens. And if you are really certain, you can write let String(s) = v to avoid nesting.

@arjanvaneersel are we really certain here or should we consider other cases? I think in this particular case we can be certain 😅

Line 67

Why can you approve only a single address?

I think here it's really about being close to the well known Ethereum based ERC-721 standard and just not overcomplicate things

Line 68

Didn't you want to use set from Set.aes maybe?

why do you think a set makes sense here @radrow? do you mean the value where currently bool is used and maybe not even needed? I am currently questioning why we need the bool. I see that in the Ethereum implementation but why would we wanna keep track of deactivated operaters, can't we just remove?

Line 182

Why do we need to specify from as it comes from the state?

this is also more a thing of being close to the well known Ethereum standard. I don't think there is another reason - except in requiring explicites input from the caller in that regards

Line 217

Naming inconsistency: functions owner and get_approved

not sure if your statement is valid. what problem do you see here? @radrow

Line 103

You don't check for consistency with metadata_type, but the rest of the code makes assumptions on that

yeah, the type is defined on contract level right now. so we should make sure that this cannot be mixed up when minting.

@arjanvaneersel would you mind adjusting the code? :)

Line 82

What is balance really used for? As I look it stores the amount of tokens one has, which can be transfered to some map that is never read

it's used to return the amount of NFTs one owns in this specific contract. it's being used here:

aex141-examples/contracts/swappable.aes

Line 162 in 26a553e

entrypoint balance(owner: address) : option(int) =

do you have an alternative suggestion? not sure if I get your point here 😅

[radrow]

is Call.caller more expensive? or just a convenience thing?

It's just convenience and being explicit. But that was more a suggestion than a concern.

why would we want Map(map(string, string))? can you elaborate on that one?

Map(string, string) is a data constructor called Map that stores two strings. It's totally isomorphic to a tuple (string * string). As I read through the code and comments, I guessed that you wanted an arbitrary string-to-string storage instead. If that's the case, the data constructor should contain a map from string to string, not two strings as it is done now.

I just wanted to make sure that you did what you really intended.

why do you think a set makes sense here

As I see, you need a unique collection here (correct me if I am wrong). In the current state however, you distinguish three states of an address:

1. It is not there
2. It is there and "true"
3. It is there and "false"

As I look into the code, cases 1 and 3 are equivalent which creates a confusing redundancy. An example distinction could be whether an address has ever been an operator, but I don't see that information being used (nor useful).

set('a) is just a wrapper over map('a, unit) but with a neater interface.

Why do we need to specify from as it comes from the state?

this is also more a thing of being close to the well known Ethereum standard. I don't think there is another reason - except in requiring explicites input from the caller in that regards

A thing that came to my mind is that it allows you to assert who is the owner of the NFT at the time of making the transaction -- that can save you from making an unwanted transfer if there is some data race and the token gets transfered after someone transfers it to someone else, who you happen to operate.

not sure if your statement is valid. what problem do you see here?

That to get approved you use get_approved, but to get owner you use owner instead of get_owner

it's used to return the amount of NFTs one owns in this specific contract. it's being used here

I just didn't know why anybody would want this information. Also, why not return the whole list then? Plus, I think if an address has no tokens, it should rather return 0 than sometimes 0 and sometimes none (meaning that I find option unnecessary)

[marc0olo]

thanks for all the valuable feedback @radrow - the swappable extension has been dropped at least temporarily. also we had a lot of discussions and the latest state of the contracts can be found here in case you wanna provide further feedback:

• https://github.com/aeternity/aex141-nft-collection-example/blob/e4d8190519d8a2ac3381f0ecd753b3716a030ce1/contracts/CollectionUniqueNFTs.aes
• https://github.com/aeternity/aex141-nft-collection-example/blob/e4d8190519d8a2ac3381f0ecd753b3716a030ce1/contracts/CollectionTemplateEditionNFTs.aes

a lot of your feedback has been incorporated there. an additional review of these two contracts would be awesome. @bogdan-manole already had a quick look. but maybe also @ghallak and @hanssv wanna have some look at these two new examples mentioned above.