feat(NFT): add events to standard #627

willemneal · 2021-11-04T16:02:25Z

This borrows the types for NFT event standard used by indexer. The only real change is that serializers are implemented for these types and serde_with is used to not serialize optional fields.

Perhaps these types should live in a separate repo that is shared between the sdk and the indexer, but I thought I'd put them here for now.

willemneal · 2021-11-08T20:54:23Z

@austinabell @ChaoticTempest This is a rough start. I am a fan of this method since it seems to add a lot of code, so I would love some feedback.

ChaoticTempest · 2021-11-12T21:14:50Z

yeah, would be nice to have a common crate for all these types. Let me see if I can't get one up, but let me workshop around a name. near-primitives is already taken. near-commons could be one but might be confusing with near-primitives

willemneal · 2021-11-13T19:48:14Z

@ChaoticTempest How about near-standards-primitives?

willemneal · 2021-11-18T14:14:24Z

@ChaoticTempest any luck?

ChaoticTempest · 2021-11-19T16:53:14Z

@willemneal so was talking to @austinabell about this, but it seems like this would be better still living here since most of this is just the NFT related types. If anything, we'd surface NearEvent into lib.rs where we can potentially populate it with other events too like FT. What do you think?

willemneal · 2021-11-19T18:31:06Z

Sounds good to me. I'll go ahead and update it. @frol @telezhnaya What are your thoughts for adding near-contract-standards as a dep to the indexer so that we can share the types?

frol

What are your thoughts for adding near-contract-standards as a dep to the indexer so that we can share the types?

We would need to evaluate the dependencies tree before making this decision, but I think it should be fine. It is definitely not a priority at the moment, though. We will get back to it after FT events are implemented.

near-contract-standards/src/non_fungible_token/event.rs

frol · 2021-11-20T21:11:05Z

near-contract-standards/src/non_fungible_token/event.rs

+    pub fn log_nft_mint(owner_id: String, token_ids: Vec<String>, memo: Option<String>) {
+        NearEvent::nft_mint(vec![NftMintData { owner_id, token_ids, memo }]).log();
+    }
+    pub fn log_nft_transfer(
+        old_owner_id: String,
+        new_owner_id: String,
+        token_ids: Vec<String>,
+        memo: Option<String>,
+        authorized_id: Option<String>,
+    ) {
+        NearEvent::nft_transfer(vec![NftTransferData {
+            authorized_id,
+            old_owner_id,
+            new_owner_id,
+            token_ids,
+            memo,
+        }])
+        .log();
+    }
+
+    pub fn log_nft_burn(
+        owner_id: String,
+        authorized_id: Option<String>,
+        token_ids: Vec<String>,
+        memo: Option<String>,
+    ) {
+        NearEvent::nft_burn(vec![NftBurnData { owner_id, authorized_id, token_ids, memo }]).log();
+    }


This API has a few problems:

The usage of these methods will be error-prone to use and hard to reason about:

NearEvent::log_nft_transfer("frol.near", "willem.near", vec!["dacha".to_string()], Some("qq".to_string()), Some("alex.near")) NearEvent::log_nft_burn("willem.near", Some("alex.near"), vec!["dacha".to_string()], Some("qq".to_string()))

(You already mixed up the order of authorized_id in transfer and burn methods)

These methods promote non-batched events and thus we promote using more gas on events than necessary

Yeah oops, this was from just following the ordering from the struct.

I agree, my inital thoughts were that they were the common case so it should be easy to use.

I fixed 1 and added methods to log data vectors.

near-contract-standards/src/non_fungible_token/event.rs

frol · 2021-11-20T21:21:21Z

near-contract-standards/src/non_fungible_token/event.rs

+#[skip_serializing_none]
+#[derive(Serialize, Deserialize, Debug)]
+pub struct NftMintData {
+    pub owner_id: String,
+    pub token_ids: Vec<String>,
+    pub memo: Option<String>,
+}
+
+#[skip_serializing_none]
+#[derive(Serialize, Deserialize, Debug)]
+pub struct NftTransferData {
+    pub authorized_id: Option<String>,
+    pub old_owner_id: String,
+    pub new_owner_id: String,
+    pub token_ids: Vec<String>,
+    pub memo: Option<String>,
+}
+
+#[skip_serializing_none]
+#[derive(Serialize, Deserialize, Debug)]
+pub struct NftBurnData {
+    pub authorized_id: Option<String>,
+    pub owner_id: String,
+    pub token_ids: Vec<String>,
+    pub memo: Option<String>,
+}


We may consider using Cow<'a, str> instead of Strings to avoid unnecessary clones when users would need to generate the events without giving the ownership over the strings. Yet, that might make the API harder to follow.

Yeah again this is a copy/paste and I agree that we should try to minimize the cost of these logs.

Still not sure how to fix this. It is annoying with to_strings everywhere.

near-contract-standards/src/non_fungible_token/event.rs

danielwpz · 2021-11-24T06:52:49Z

guys any luck with this PR?

willemneal · 2021-11-30T23:53:08Z

@danielwpz About to push up some changes!

willemneal · 2021-12-01T18:34:25Z

@ChaoticTempest @frol @austinabell @BenKurrek

I moved everything up a level and added three new methods for logging batches of events. However, I'm still not a fan of String types everywhere. Do you think it's worth changing the API to Cow as @frol suggested?

austinabell · 2021-12-01T18:45:13Z

@ChaoticTempest @frol @austinabell @BenKurrek

I moved everything up a level and added three new methods for logging batches of events. However, I'm still not a fan of String types everywhere. Do you think it's worth changing the API to Cow as @frol suggested?

Might be worth at least switching and seeing how it affects code size. Keep in mind, by default Cow will deserialize as an owned string and you don't get the performance of referencing the deserialized buffer so this just saves an allocation on initialization.

As for this PR and the other, I think it might have been assumed this was stale as it didn't have the connection to standards, not sure. Hopefully, we can merge the efforts somehow to get the best of both :)

willemneal · 2021-12-01T19:26:47Z

What do you mean as it didn't have the connection to standards?

austinabell · 2021-12-01T20:18:48Z

What do you mean as it didn't have the connection to standards?

it isn't used in the NFT standard, this just adds the types, which is fine

ChaoticTempest

I'll leave it up to @austinabell to have the final say, but overall this PR looks good to me. The sim tests will have to be moved over to workspaces in the future, but for now we can leave that for a later PR. Think we need a CHANGELOG entry as well

near-contract-standards/src/event.rs

ChaoticTempest · 2022-01-20T01:19:25Z

near-contract-standards/src/event.rs

+#[derive(Serialize, Deserialize, Debug)]
+pub struct NftMintData<'a> {
+    #[serde(borrow)]
+    pub owner_id: Cow<'a, str>,


Maybe worth putting in the comments somewhere in this file about why the optimization with Cow's are needed, or linking back to this PR with the discussions could work too

Co-authored-by: Phuong Nguyen <ChaoticTempest@users.noreply.github.com>

austinabell

Sorry for long time to review, think there is a slight miscommunication about who owns standards

near-contract-standards/src/event.rs

austinabell · 2022-01-20T18:40:38Z

near-contract-standards/src/event.rs

+    pub old_owner_id: Cow<'a, str>,
+    #[serde(borrow)]
+    pub new_owner_id: Cow<'a, str>,
+    #[serde(borrow)]
+    pub token_ids: Vec<Cow<'a, str>>,
+    #[serde(borrow)]
+    pub authorized_id: Option<Cow<'a, str>>,
+    #[serde(borrow)]
+    pub memo: Option<Cow<'a, str>>,


Is there a benefit to exposing all of these? Seems like we are just adding too much surface area to maintain

This was from the original code from the indexer. So no real reason to expose them.

@frol @telezhnaya Thoughts?

austinabell · 2022-01-20T18:40:57Z

near-contract-standards/src/event.rs

+
+#[skip_serializing_none]
+#[derive(Serialize, Deserialize, Debug)]
+pub struct NftTransferData<'a> {


Would be nice if there were docs on some of these, but that can always be added in follow up PR

austinabell · 2022-01-20T18:43:15Z

near-contract-standards/src/event.rs

+    }
+
+    pub(crate) fn to_json_string(&self) -> String {
+        serde_json::to_string(self).unwrap()


this should never fail, so would be slightly more ideal to just panic through host function to avoid file-specific data being included. Fine to have this done in a following PR though.

near-contract-standards/src/event.rs

near-contract-standards/src/non_fungible_token/core/core_impl.rs

near-contract-standards/src/event.rs

willemneal · 2022-01-20T20:25:41Z

@austinabell I addressed most of your concerns. I didn't change the visibility of the inner fields.

I also improved the API so that AccountId's are used directly ensuring that only valid ones are written to the event ( unless they do new_unchecked).

Removing cow reduced the size of the final contract by around 3K, so not too much code bloat, but still an improvement.

PTAL and let me know.

Co-authored-by: Austin Abell <austinabell8@gmail.com>

willemneal · 2022-01-24T22:11:01Z

@austinabell Could really use this in the wild! Also the FT events have been added to the standard to so I'll add that in a future PR.

austinabell

Feel free to change the things I've commented on. Going to follow this to make some other changes wrt what of the API is exposed to make it easier to maintain anyway so nbd if not

near-contract-standards/src/event.rs

austinabell · 2022-01-25T00:09:45Z

near-contract-standards/src/event.rs

+    pub old_owner_id: &'a str,
+    #[serde(borrow)]
+    pub new_owner_id: &'a str,


why not these as account ids?

The AccountId references are used in the constructors. If I add them here I get:

the trait bound `&'a AccountId: Deserialize<'_>` is not satisfied the following implementations were found: <AccountId as Deserialize<'de>>

yeah, the point before is that the deserialize isn't needed for anything. nbd can leave as is

willemneal · 2022-01-25T14:58:23Z

@austinabell @ChaoticTempest Thanks for reviewing, but it seems I have lost my ability to hit the final merge.

austinabell · 2022-01-25T15:11:18Z

@austinabell @ChaoticTempest Thanks for reviewing, but it seems I have lost my ability to hit the final merge.

Because of the conflicts? Was just waiting for that

willemneal · 2022-01-25T15:13:39Z

Literally not an option to merge for me. I guess you have to be in the NEAR org to get that power.

austinabell · 2022-01-25T15:22:54Z

Literally not an option to merge for me. I guess you have to be in the NEAR org to get that power.

Nope, just needed to be merged manually. I couldn't have pulled it in either. Hope you don't mind me pushing to your branch to fix

willemneal · 2022-01-25T15:31:10Z

No issues just glad to see it wrapping up.

frol · 2022-02-01T11:52:40Z

@willemneal I am so happy to see how this turned out to be a nice API! I want to thank you for driving this! It is really tiresome work to get to perfection. I also apologize that I failed to provide the feedback in timely manner 🙏

@miraclx Could you maybe take a look into why the trait bound &'a AccountId: Deserialize<'_> is not satisfied: #627 (comment) ?

austinabell · 2022-02-01T15:57:29Z

@willemneal I am so happy to see how this turned out to be a nice API! I want to thank you for driving this! It is really tiresome work to get to perfection. I also apologize that I failed to provide the feedback in timely manner 🙏

@miraclx Could you maybe take a look into why the trait bound &'a AccountId: Deserialize<'_> is not satisfied: #627 (comment) ?

No worries, the API was updated in dce4bd8 so there is no need for this. Deserializing into an AccountId reference is intended given our current AccountId API, so no need for @miraclx to look into anything here I don't think

feat(NFT): add events to standard

a6e16e8

willemneal force-pushed the feat/event_logs branch from 207eeaf to a6e16e8 Compare November 4, 2021 16:05

willemneal mentioned this pull request Nov 4, 2021

NFT doesn't show up when initially minted. near/near-wallet#2226

Closed

willemneal added 3 commits November 8, 2021 14:17

Merge remote-tracking branch 'origin/master' into feat/event_logs

3238e4a

feat: add last two events and add tests

b65081a

fix: fmt

19151a1

willemneal marked this pull request as ready for review November 8, 2021 20:52

willemneal requested review from austinabell, ChaoticTempest, evgenykuzyakov and mikedotexe as code owners November 8, 2021 20:52

fix: use near indexer's types

4b8f0aa

frol reviewed Nov 20, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into feat/event_logs

d27974f

willemneal added 2 commits December 1, 2021 11:15

feat: add new methods; address feedback and move file up a directory

c32de7a

chore: update wasm binaries

a32852b

willemneal mentioned this pull request Dec 1, 2021

NFT Events Standard #652

Closed

fix: fmt

1ddc497

Merge branch 'master' into feat/event_logs

3459d27

ChaoticTempest reviewed Jan 20, 2022

View reviewed changes

willemneal and others added 2 commits January 20, 2022 15:51

Update near-contract-standards/src/event.rs

1404f55

Co-authored-by: Phuong Nguyen <ChaoticTempest@users.noreply.github.com>

chore: add more documentation and edit CHANGELOG

1746487

willemneal requested a review from ChaoticTempest January 20, 2022 15:04

austinabell reviewed Jan 20, 2022

View reviewed changes

willemneal added 3 commits January 20, 2022 14:57

feat: switch to &'a str since the event will have shorter lifetime

9d0d28d

fix: update wasm

1ce258b

chore: add more documentation to core_impl

64c1c6f

austinabell reviewed Jan 20, 2022

View reviewed changes

near-contract-standards/src/event.rs Show resolved Hide resolved

near-contract-standards/src/event.rs Outdated Show resolved Hide resolved

willemneal mentioned this pull request Jan 20, 2022

Standards ownership ambiguity #705

Closed

willemneal and others added 3 commits January 20, 2022 21:30

Update near-contract-standards/src/event.rs

d004203

Co-authored-by: Austin Abell <austinabell8@gmail.com>

fix: annoying wasm files

179faf1

Merge branch 'master' into feat/event_logs

bce9abe

willemneal requested a review from austinabell January 21, 2022 14:52

willemneal mentioned this pull request Jan 21, 2022

chore: bump versions and update changelog for pre.6 release #707

Merged

Merge remote-tracking branch 'origin/master' into feat/event_logs

ae405da

austinabell approved these changes Jan 25, 2022

View reviewed changes

Merge branch 'feat/event_logs'

173f6f7

austinabell merged commit ea8c1f4 into near:master Jan 25, 2022

itegulov mentioned this pull request Jan 27, 2022

refactor: replace non-fungible token example sim tests with workspaces #702

Merged

feat(NFT): add events to standard #627

feat(NFT): add events to standard #627

Conversation

willemneal commented Nov 4, 2021 • edited Loading

willemneal commented Nov 8, 2021

ChaoticTempest commented Nov 12, 2021

willemneal commented Nov 13, 2021

willemneal commented Nov 18, 2021

ChaoticTempest commented Nov 19, 2021

willemneal commented Nov 19, 2021

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielwpz commented Nov 24, 2021

willemneal commented Nov 30, 2021

willemneal commented Dec 1, 2021

austinabell commented Dec 1, 2021

willemneal commented Dec 1, 2021

austinabell commented Dec 1, 2021

ChaoticTempest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willemneal commented Jan 20, 2022 • edited Loading

willemneal commented Jan 24, 2022

austinabell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willemneal commented Jan 25, 2022

austinabell commented Jan 25, 2022

willemneal commented Jan 25, 2022

austinabell commented Jan 25, 2022

willemneal commented Jan 25, 2022

frol commented Feb 1, 2022 • edited Loading

austinabell commented Feb 1, 2022

willemneal commented Nov 4, 2021 •

edited

Loading

willemneal commented Jan 20, 2022 •

edited

Loading

frol commented Feb 1, 2022 •

edited

Loading