NFT standard

[ailisp]

This implements all traits, functionalities and tests as in https://github.com/near/near-sdk-rs/tree/master/near-contract-standards/src/non_fungible_token.

Still missing doc strings, will be added tomorrow. But this has been so long as blocked by features, working on higher priorities, etc. I'd like to see some comments!

[volovyks]

Some initial comments:

• @ailisp have you tried to run rust tests on this resulting wasm file? (is it possible?)
• can we organize imports without paths? Maybe similarly to how it's done with main lib and examples?
• Have you tried to use it in the external project? I wander if standards should live in src, then be built to /lib and examples will live in /examples, as well as tests in main /tests folder

[ailisp]

Thanks for your review.

@ailisp have you tried to run rust tests on this resulting wasm file? (is it possible?)

Yes, __tests__/my-nft/*.ava.js and "Contract standard / examples" on ci does that.

can we organize imports without paths? Maybe similarly to how it's done with main lib and examples?

Will do!

Have you tried to use it in the external project?

I haven't. Updating the /example-contracts test to depend on a lib version should work too, I'll make it that way.

I wander if standards should live in src, then be built to /lib and examples will live in /examples, as well as tests in main /tests folder

This is a good question! In rust sdk, near-contract-standard is a separate crate. We probably want this too, as most non-standard contract doesn't need near-contract-standards. It is a challenge for me to figure out where should I organize lib, examples and tests in this case. Maybe we should turn this repo into a workspace. Maybe it should live in a separate repo. If in same repo, whether examples should be organized in root /examples or their own examples, etc. A couple of things to discuss.

So in this PR, I want only ensure it works as an external library, and let's have a discussion next week on ^ and make a separate PR to properly organize / package near-contract-standards in general!

[volovyks]

Tremendous effort @ailisp ! We can iterate on all the comments later, it can be merged now. The code is clean and well-documented, and the tests look robust.

[volovyks]

what 8 stands for? (let's add it to the comment)

[ailisp]

Good catch! I realized this is not actually 4 + 8, as we are not using borsh at all, let me figure out a right number!

[ailisp]

It's actually difficult to figure out the size to storage refund.
in rust, it's key: string, value: u64, in borsh size is 4 + string length + 8 (for uint 64). In js it's key: string, value: bigint, and it's json serialized, not able to count this way.

I'll write a different logic (by counting diff in near.storageUsed().

[volovyks]

I'm curious if we are using Rust style Option here because it's the only way to make it work the same way as in Rust standard. Can it work with ? somehow?

[ailisp]

Good point! ? is certainly more storage efficient, when it's not present it doesn't get serialized.

[volovyks]

Looks like a lot of work for somebody to use a standard token. I wander if there is a way not to override most of the functions if they remain "standard". (just a discussion for the future)

[ailisp]

Yeah, this need more discussion. In rust it's done via a few macros: impl_non_fungible_token_core etc. I didn't see actively maintainted or commonly used JS macro / code generator libraries to integrate with our build pipeline. One way maybe making a "macro" with babel, like our includeBytes

[waynenilsen]

i completely agree with @volovyks but i think that leads to deeper discussions about the codegen / transpilation process and why something simpler may not work. I think that the desirable outcome is something like this

in this repo (or some other repo) the standard exists and is implemented

@NearBindgen({ requireInit: true })
export class NonFungibleToken {
...
}

later, someone wants to customize (such as myself) and add a function, called, say, deposit to extend the abilities of the NFT as such

@NearBindgen({ requireInit: true })
export class MyNonFungibleToken extends NonFungibleToken {
    // i get everything from NonFungibleToken by default but now i can extend
   @call({ payable: true})
    deposit(args: DepositArgs): void { ... }
}

does that make sense?

we tried that but currently contracts can't be composed in this way, I don't have the errors at hand but I think a lot of it goes to the general codegen process related to the transpilation.

I can write a ticket to discuss further or we can chat in discord etc

[volovyks]

Yes, I have something like this in mind as well. @waynenilsen issues with described problematics would be great!

[ailisp]

I merged this one and created #290 . @waynenilsen feel free to experiment and may be create some proof of concept from latest develop!

[ailisp]

@volovyks Addressed those easier ones. For more difficult ones, Will try to address them next Monday. As you said it's generally good - before the sprint planning I'll move unresolved / TBD points in separate issues to merge this PR :)

[thisisjoshford]

A community dev was in office hours today and was confused with this PR's implementation of the NFT standard compared to the Rust SDK. @BenKurrek was also on the call, can you comment here?

[BenKurrek]

@volovyks @thisisjoshford From my understanding, if you pass in an array with the JS SDK, it uses positional arguments:
nft_transfer([receiver_id, token_id, approval_id, memo]).

This is not compatible with the standards and I believe we should use named arguments such as:
nft_transfer({receiver_id: string, token_id: string, approval_id: number, memo: string}).

By using positional arguments, it might not be compatible with some implementations and could break things. Please correct me if I'm wrong.

[ailisp]

Pierre Le Guen has reached me and we agree to move to
nft_transfer({receiver_id: string, token_id: string, approval_id: number, memo: string}). format of argument. I think this is the issue mentioned by @thisisjoshford and @BenKurrek . Let me know if there's other issues!

[ailisp]

Most comments are addressed. For three more challenging one, I move and track them in #288, #289, #290