Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: adding soulbound token #393
feat: adding soulbound token #393
Changes from 9 commits
365e8c2
7aa2f8b
1783a3e
515f35d
fb488a8
56397d0
41d6020
6c261e1
cb96e8d
c5c64c5
f29bfa2
764c68e
0f26b31
89aa83a
13b152c
afab9e8
e0d92fb
664573f
e1340c8
e8ade97
a4a6e5c
99e9f99
1e9a6b3
31d3ea8
08b0e2a
9f1266f
a8f4f49
8acb192
893f7a6
2d65c82
389a4bc
ad558dc
dbc20d8
9a28f96
00649b0
2213814
2ea6fef
3d809fa
6c513a6
4e300e6
e8ea5ec
643cb89
21491ae
08bd3ac
c18f2b4
99943a3
549546a
4c4231d
d2cbbdf
1762ba2
3a8ceff
786d4a1
dba3b4e
ee2317b
54e9a9d
07021ea
34fc893
876d9d3
808e047
f76f85d
45b73c9
670cf5d
dd7ebc2
8a7c584
066d53b
ba4a5db
8b91fb7
098d89c
9104c0e
b7c4d44
7850125
64afc80
0ec0dca
c57a4d2
eece830
b816793
57b4ac6
3a410f9
1f5001e
673beb6
3fc3447
1149e98
66c794f
fe352b8
7e784a2
06279c5
51b14d9
ad56e5f
3ea249b
9f4c86e
7b753f1
912803f
ceabc86
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may consider
nft
prefix where relevant to make the functions compatible with existing tools. eg :nft_tokens_for_owner
instead ofsbt_tokens_for_owner
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collision with the existing tooling will create more harm than help here as we already discussed in #359. The only way I can see that being resolved is by the NEP which will help to identify the supported extensions, read more in #129, #154, and #275.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was trying to refer to #351, which could be then used as a requirement for SBT NEP to unlock
nft_
prefixes by saying that SBT = NEP-171 + NEP-330 + NEP-XXX (that just defines thatnft_transfer
is not available) - this way the tooling can reasonably handle the deviation from NEP-171 through introspection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was bringing up interface registration already in 2020, then created #154 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we reuse
nft_
prefix then SBT is a subset of NFT, not an extension. Adding #330 makes sense. We need to discuss more if we want to be compatible with NFT standard, and carry over some of the shortcoming (like representing the token id as aU64
rather thanu64
-- more about this in Considerations section)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know that all implementations of this will require 5 milliNEAR to cover storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I will rephrase it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are sure that seconds is a good idea here? The NFT metadata standard mandates milliseconds and you can this here in lines 51-54. I propose to keep it consistent. I also suggest we use either milliseconds or nanoseconds, as these are provided as functions from near-sdk-rs. With NFTs, most of the old contracts just use nanoseconds instead of the standardized milliseconds, same will probably happen here. Esp. in case of nanoseconds
U64
orU128
data type might be more appropriate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Milliseconds most probably were inherited from the JS environment, and nanoseconds are used in NEAR Protocol for timestamps as seconds precision was not enough and then given that
u32
was not enough to hold even seconds precision for long enough,u64
was enough even to have nanoseconds precision.Unfortunatelly, NFT NEP and near-contract-standards implementation are currently in disagreement:
https://github.com/near/near-sdk-rs/blob/89f8c2d776c8d00feb45239fdb15725527f1894b/near-contract-standards/src/non_fungible_token/metadata.rs#L36
https://github.com/near/NEPs/blob/550e428cfffafaa28bd84f2e882504a8679d01b6/neps/nep-0177.md
There is clearly a need for a decision regarding time references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer
seconds
, as the most natural, and good enough metric (unix timestamp measure, SI unit).Personally, I think use of nano-seconds in NEAR is over-engineering.
However, if the ecosystem settled on milliseconds, then we can use milliseconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 2 cents:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NEAR VM provides
block_timestamp
in nanoseconds, so it is usually a no-brainer to use that value to compare it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nanoseconds
are not user friendly. I don't believe anyone things in terms of nanoseconds when definingexpires_at
. Most of the people will think in terms of days or hours for things related to human. Seconds are a good Unix standard. Milliseconds are default in JS.Even if there are 100 blocks per second - the condition will be well defined. it's event the last transaction from the given block if using <= or the last transaction from the previous block if using < .
I don't believe anyone will like to go to nanosecond precision for things like
SBT.expires_at
. I other words, why anyone would put expires at:April 7 2023 11:23:56.751209857
?Also, worth to note that the block unit is a sequence number (1,2,3 ...).
With that, I think
second
(as more human friendly) ormillisecond
(as JS default) are definitely more appropriate. My preference isseconds
. However, if there is broader adoption formilliseconds
then let's use milliseconds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on seconds/milliseconds/nanosecond for the expiration timestamp. There are pros and cons to each of them. Just make sure you align the whole document on using one selected option as currently
TokenMetadata
states that it is milliseconds precision.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reasoning to call this revoke instead of burn?
Also, the
SbtRevokeLog
does not show up inSbtEventLogData
and there is nosbt_revoke
event name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed
SbtRevoke
and updated to useNEP-171 NftBurn
instead and cleaned the interface. So we will have few "native" events, and 2 inherited fromNEP-171 NftBurn
. Note that we still need to discuss & agree the level of compatibility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new SBT Registry update, now we don't use NEP-171 events any more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As others have pointed out, mixing these standards and making interfaces compatible is well-intentioned but doomed to cause problems. There are so many examples where NFT standards are poorly or improperly implemented, adding another standard with differing functionality but equal naming in there will cause lots of misclassifications between NFTs/SBTs, and then recover methods called on NFT contracts, SBTs attempted to be listed on NFT marketplaces and so on. Probably nothing will happen because of smart contract panics, but devs will have to put defensive measures into place that will impair interoperability efforts. Users will be annoyed and blame devs all over the place. Everyone will be unhappy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment. On one hand side NEP-171 is desirable (support from existing tooling), on the other, it has some short commings. I've updated teh sentence and added more about it to the Considerations section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robert-zaremba Existing tooling will inevitably need to have dedicated support for SBTs, and the role of the standards is to make the support hassle-free. Thus, as we already covered before, it is better to avoid any overlap with the NFT ecosystem to make everybody's lives easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I still think that some developers may find it desirable to implement NFT query API on the issuer level. It is possible, so I would keep it mentioned. I will reword the sentence to make it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reasoning as above. New events. Updating indexers is not as bad as tokens being misclassified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. But it's negative from the indexer perspective: new support will have to be added.