Enforce Standard Logging Compliance by Abstraction #934

coleFD · 2022-10-07T13:08:43Z

The goal of this commit is to force compliance of standard logging practices so more efficient indexing features can be developed in the future such as a logs bloom filter by event name. See the original discussion here: near/NEPs#297

! NOTE:
Please see discussion in the linked issue above to determine if it would be better to make the standard and version field optional

! BREAKING CHANGE:
For this change to be useful, we will need to privatize/remove the env::log() and env::log_str() functions, which has not been implemented yet in this commit. May not be a breaking change, but adding this to this section, so it is recognized

near-sdk/src/environment/env.rs

coleFD · 2022-10-10T14:01:38Z

Changed to WIP because I would like to convert to a single derive attribute with kwargs, and I need to resolve issues with lifetimes. I also still need to privatize env::log(), env::log_str(), and remove log!(). Any feedback on this pattern (see ./src/types/events.rs test for what it would look like in a contract). Also, I am not sure if it is better to have 2 functions to emit events or just have one, but I like the Event::emit() option since it's easier to spot when skimming code.

Another possible idea is to add it to the near_bindgen attribute, so like near_bindgen(event), and events could be added to the abi a future issue??

frol · 2022-10-11T11:54:34Z

@coleFD Exposing events via ABI would be awesome! I feel it is a much heavier lift than the initial idea, but I see it as a cool challenge 😃

coleFD · 2022-10-11T13:48:25Z

near_bindgen(event)

For sure, I dont think adding events to the abi should be added to this issue. I dont want to add to many changes into 1 issue, but If I utilize near_bindgen instead of creating a new macro, it will put us in a better spot to add the abi functionality in the future.

coleFD · 2022-10-12T15:43:42Z

The implementation is done, but I still need to remove the previous logging functions. I do not know what that process should look like since it will affect all of the examples and smart-contract-standards contracts as well. I can update these as needed, but getting approval first would be better. We could even create another issue for the transition..

near-sdk-macros/src/core_impl/event/mod.rs

frol · 2022-10-13T12:52:57Z

I still need to remove the previous logging functions.

Please, clarify which logging functions you refer to.

frol · 2022-10-13T12:53:38Z

@austinabell Could you recommend someone to review this PR?

coleFD · 2022-10-13T13:07:52Z

I still need to remove the previous logging functions.

Please, clarify which logging functions you refer to.

The goal of this is to prevent logging without having the standard metadata fields standard, version, event, and data, so any logging functionality that does not do that should be removed from the users' toolbox in my opinion. I think the only functions that do this are env::log(), env::log_str(), and the macro log!().

Are there any concerns with doing this?

frol · 2022-10-13T16:58:09Z

log_str and its derivatives should be left as is. There is no problem with using logs in contracts, though, we should definitely promote usage of events. Also, I would imagine we reimplement NFT and FT events using this facility. I see that @austinabell is out of office until next week, so we will need to wait until he gets back and gets time to review it.

frol · 2022-10-17T13:14:03Z

By the way, I have just learned about this prior art: https://github.com/NEARFoundation/near-contract-tools#events

coleFD · 2022-10-17T13:18:53Z

By the way, I have just learned about this prior art: https://github.com/NEARFoundation/near-contract-tools#events

interesting, is there still incentive to add this to near-sdk?

frol · 2022-10-17T13:25:04Z

I would like to hear what @austinabell thinks

austinabell

Thanks for working on this! I think this can be useful to have in the SDK somewhere (unsure if it should live in near_sdk or near_contract_standards crate, I'll think about that more.

@itegulov wdyt about this? Do you think the implications of enum vs struct annotation would have any implication on integrating into the ABI?

austinabell · 2022-10-17T14:29:53Z

near-sdk/src/types/event.rs

+ #[near_bindgen(events)]
+ pub enum TestEvents<'a, 'b, T>
+ where
+ T: crate::serde::Serialize + Clone,
+ {
+ #[event_meta(standard = "swap_standard", version = "1.0.0")]


Curious why the decision to group all events in one enum? Would this not just be simpler if it was an attribute on a struct? I suppose this is a bit opinionated, but wondering if there is a reason this must be an enum

I went this route based on frol's comment above. It looked clean as an enum since it grouped events together. I can convert it to implement structs if we decide to go that route

nope, that's fine. It's opinionated, was just sharing my intuition. I suppose we could also just support both if it doesn't increase complexity meaningfully.

cc @itegulov wdyt?

I treat events as some sort of standard and an enum in my head represents the fact that there is a finite number of types of events that a contract can emit. That said, I would consider extracting standard = "..." to the enum declaration level instead of the enum variants level. This way we could have FtEvents, NftEvents, MtEvents, FayyrEvents, etc, so one could import and use them in a composable way.

This is the last thing I need clarification on before committing the new changes. Should I move forward with keeping this as an enum and moving standard to the enum declaration level?

@itegulov wdyt about this? Do you think the implications of enum vs struct annotation would have any implication on integrating into the ABI?

I don't think it matters, we should be able to generate ABI for both options.

Personally, I find that having a mega-enum covering all standards used in this contract is a bit of an antipattern, but not a strong opinion. What @frol suggested makes more sense to me - having an enum per standard looks cleaner.

near-sdk-macros/src/lib.rs

austinabell · 2022-10-17T14:34:02Z

near-sdk/src/types/event.rs

@@ -0,0 +1,95 @@
+pub trait StandardEvent {
+ fn format(&self) -> String;
+ fn emit(&self);


This could have a default implementation to prevent devs from having to implement it manually if they choose.

Suggested change

fn emit(&self);

fn emit(&self) {

crate::env::log_str(&format!("EVENT_JSON:{}", &standard_event.format()));

}

near-sdk/src/types/event.rs

mooori · 2022-10-31T10:27:08Z

Using darling to parse attributes passed to #[near_bindgen] could help to remove custom parsing logic.

Another benefit of using darling is related to supporting other attributes besides events. That could be done by adding fields to the structs into which attributes are parsed, instead of adding more custom parsing logic.

For example, this PR shows how darling could be used for near_bindgen and handling multiple attributes might look like this.

near-sdk-macros/src/lib.rs

near-sdk/src/types/event.rs

near-sdk-macros/src/lib.rs

…ixes

…Ref<str>

coleFD · 2022-11-03T13:04:48Z

any feedback on the latest commit?

austinabell · 2022-11-03T17:01:38Z

any feedback on the latest commit?

There's actually a much simpler way to make sure there is no overlap here. I made the commit 1e6c09e.

Feel free to cherry-pick, make a similar change yourself, or let me know if I can push it to this branch. It's a small nit but with this, you can still have a collision when you don't need that to be the case.

coleFD · 2022-11-03T17:16:02Z

any feedback on the latest commit?

There's actually a much simpler way to make sure there is no overlap here. I made the commit 1e6c09e.

Feel free to cherry-pick, make a similar change yourself, or let me know if I can push it to this branch. It's a small nit but with this, you can still have a collision when you don't need that to be the case.

Nice! you can push it to this branch!

austinabell · 2022-11-03T17:46:42Z

So when making that other change, it wasn't clear to me why the trait is necessary at all. Seems unergonomic to require devs to import that trait to use this. Why not just remove it like in 1cc5c19 ?

austinabell · 2022-11-03T17:54:55Z

Also, ignore CI failure, it's because of a Rust release. #955 fixes it

coleFD · 2022-11-03T17:55:32Z

So when making that other change, it wasn't clear to me why the trait is necessary at all. Seems unergonomic to require devs to import that trait to use this. Why not just remove it like in 1cc5c19 ?

initially the plan was to have a StandardEvent trait to standardize the interface for all current and future serialization formats. I agree it doesnt make much sense if we just have json

austinabell · 2022-11-03T18:00:15Z

For my first iteration, the plan was to have a StandardEvent trait to standardize the interface for all current and future serialization formats. I agree it doesn't make much sense if we just have JSON

I just don't see the benefit in having a trait for this if all it's going to do is add the convenience env::log_str("EVENT_JSON:..."). Can always add the trait back later, but I'd rather keep the API as slim as possible.

I'll push that change to this branch then since it seems like you're on board. Would be nice to have a macro test for the codegen, since I was able to make both changes without any tests failing, which is not ideal. This is not required though, improved tests can come after this PR comes in. Sorry for so much back and forth, this is just an important interface and it would be painful to migrate later.

frol · 2022-11-04T13:17:19Z

@austinabell @coleFD Thanks for pushing it forward! I was out of the office, and I just had a chance to sync back. I see there was a huge progress! 🙏

austinabell

LGTM but would be good to have another approval since I was the last to make a commit on this branch

frol

Overall, looks good to me. I only left a few nits in the comments

frol · 2022-11-04T13:49:54Z

near-sdk/src/types/mod.rs

+#[cfg(test)]
+mod event_tests;


Is it actually the right place for the tests? Should we move it to near-sdk/tests?

near-sdk-macros/src/lib.rs

near-sdk-macros/src/core_impl/event/mod.rs

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

austinabell · 2022-11-08T13:26:30Z

thank you! Sorry for all the back and forth

coleFD · 2022-11-08T13:44:12Z

No worries! thank you guys for being responsive and taking the time to dig into it!

coleFD requested review from austinabell, ChaoticTempest and itegulov as code owners October 7, 2022 13:08

coleFD mentioned this pull request Oct 7, 2022

We need separate Events standard [NEP] near/NEPs#297

Closed

frol reviewed Oct 7, 2022

View reviewed changes

near-sdk/src/environment/env.rs Outdated Show resolved Hide resolved

frol mentioned this pull request Oct 8, 2022

Events or there alternative? Getting notified when coins have arrived to a contract near/NEPs#403

Closed

coleFD changed the title ~~Enforce Standard Logging Compliance by Abstraction~~ WIP: Enforce Standard Logging Compliance by Abstraction Oct 10, 2022

frol marked this pull request as draft October 11, 2022 11:51

coleFD force-pushed the feat/enforced-event-standard-logging branch 3 times, most recently from e486714 to 2a548aa Compare October 12, 2022 15:40

coleFD marked this pull request as ready for review October 12, 2022 15:40

frol reviewed Oct 13, 2022

View reviewed changes

near-sdk-macros/src/core_impl/event/mod.rs Outdated Show resolved Hide resolved

near-sdk-macros/src/core_impl/event/mod.rs Outdated Show resolved Hide resolved

coleFD force-pushed the feat/enforced-event-standard-logging branch 2 times, most recently from bc1c77a to baa178d Compare October 13, 2022 13:14

coleFD changed the title ~~WIP: Enforce Standard Logging Compliance by Abstraction~~ Enforce Standard Logging Compliance by Abstraction Oct 13, 2022

coleFD force-pushed the feat/enforced-event-standard-logging branch from baa178d to f263fba Compare October 17, 2022 13:50

austinabell reviewed Oct 17, 2022

View reviewed changes

near-sdk/src/types/event.rs Outdated Show resolved Hide resolved

mooori mentioned this pull request Oct 31, 2022

feat: Blacklist attributes not to forward to ContractExt methods #952

Closed

austinabell reviewed Oct 31, 2022

View reviewed changes

austinabell mentioned this pull request Oct 31, 2022

Events support near/near-workspaces-rs#225

Open

coleFD added 6 commits October 31, 2022 16:45

feat: enforce dev compliance of logging standard by abstraction

1fef2f6

feat: Changed abstraction api

3d2dc41

feat: 3rd iteration. Moved standard to enum declaration and minor f…

df36b38

…ixes

rebase

b415885

feat: changed StandardEvent to EventJson and Display type bound to As…

51b59cf

…Ref<str>

fix: add support for multiple event enums in the same file

6eecebe

coleFD force-pushed the feat/enforced-event-standard-logging branch from 00dffda to 6eecebe Compare October 31, 2022 20:52

refactor: move codegen type inside impl scope

1e6c09e

remove EventJson trait

1cc5c19

Merge branch 'master' into feat/enforced-event-standard-logging

3114a3a

austinabell approved these changes Nov 4, 2022

View reviewed changes

frol approved these changes Nov 4, 2022

View reviewed changes

austinabell and others added 3 commits November 7, 2022 19:57

Update near-sdk-macros/src/lib.rs

9413fb4

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

Update near-sdk-macros/src/core_impl/event/mod.rs

f0b5fcf

Co-authored-by: Vlad Frolov <frolvlad@gmail.com>

move test

b74e2d6

austinabell merged commit 557c22c into near:master Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce Standard Logging Compliance by Abstraction #934

Enforce Standard Logging Compliance by Abstraction #934

coleFD commented Oct 7, 2022

coleFD commented Oct 10, 2022 •

edited

Loading

frol commented Oct 11, 2022 •

edited

Loading

coleFD commented Oct 11, 2022

coleFD commented Oct 12, 2022

frol commented Oct 13, 2022

frol commented Oct 13, 2022

coleFD commented Oct 13, 2022 •

edited

Loading

frol commented Oct 13, 2022

frol commented Oct 17, 2022

coleFD commented Oct 17, 2022

frol commented Oct 17, 2022

austinabell left a comment •

edited

Loading

austinabell Oct 17, 2022

coleFD Oct 17, 2022

austinabell Oct 17, 2022

frol Oct 19, 2022

coleFD Oct 20, 2022

itegulov Oct 21, 2022

austinabell Oct 17, 2022

mooori commented Oct 31, 2022

coleFD commented Nov 3, 2022

austinabell commented Nov 3, 2022

coleFD commented Nov 3, 2022

austinabell commented Nov 3, 2022

austinabell commented Nov 3, 2022

coleFD commented Nov 3, 2022 •

edited

Loading

austinabell commented Nov 3, 2022

frol commented Nov 4, 2022

austinabell left a comment

frol left a comment

frol Nov 4, 2022

austinabell commented Nov 8, 2022

coleFD commented Nov 8, 2022

- fn emit(&self);
+ fn emit(&self) {
+ crate::env::log_str(&format!("EVENT_JSON:{}", &standard_event.format()));
+ }

		#[cfg(test)]
		mod event_tests;

Enforce Standard Logging Compliance by Abstraction #934

Enforce Standard Logging Compliance by Abstraction #934

Conversation

coleFD commented Oct 7, 2022

coleFD commented Oct 10, 2022 • edited Loading

frol commented Oct 11, 2022 • edited Loading

coleFD commented Oct 11, 2022

coleFD commented Oct 12, 2022

frol commented Oct 13, 2022

frol commented Oct 13, 2022

coleFD commented Oct 13, 2022 • edited Loading

frol commented Oct 13, 2022

frol commented Oct 17, 2022

coleFD commented Oct 17, 2022

frol commented Oct 17, 2022

austinabell left a comment • edited Loading

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

Choose a reason for hiding this comment

mooori commented Oct 31, 2022

coleFD commented Nov 3, 2022

austinabell commented Nov 3, 2022

coleFD commented Nov 3, 2022

austinabell commented Nov 3, 2022

austinabell commented Nov 3, 2022

coleFD commented Nov 3, 2022 • edited Loading

austinabell commented Nov 3, 2022

frol commented Nov 4, 2022

austinabell left a comment

Choose a reason for hiding this comment

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented Nov 8, 2022

coleFD commented Nov 8, 2022

coleFD commented Oct 10, 2022 •

edited

Loading

frol commented Oct 11, 2022 •

edited

Loading

coleFD commented Oct 13, 2022 •

edited

Loading

austinabell left a comment •

edited

Loading

coleFD commented Nov 3, 2022 •

edited

Loading