gnolang / gno

Gno: An interpreted, stack-based Go virtual machine to build succinct and composable apps + gno.land: a blockchain for timeless code and fair open-source.
https://gno.land/
Other
899 stars 377 forks source link

chore(p/grc721): Distinct Event Types for GRC721 Functions #3102

Closed r3v4s closed 1 week ago

r3v4s commented 2 weeks ago

Description

Current event(emit) code in p/grc721 really doesn't emits event.

Therefore, modified code to emit the events.

And similar to #2749, made event type for each function to be unique.

Contributors' checklist... - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests
codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.77%. Comparing base (4f27a57) to head (36b767a). Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3102 +/- ## ========================================== - Coverage 63.77% 63.77% -0.01% ========================================== Files 548 548 Lines 78681 78681 ========================================== - Hits 50180 50177 -3 - Misses 25117 25122 +5 + Partials 3384 3382 -2 ``` | [Flag](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | Coverage Δ | | |---|---|---| | [contribs/gnodev](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `61.16% <ø> (+0.62%)` | :arrow_up: | | [contribs/gnofaucet](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `14.82% <ø> (ø)` | | | [gno.land](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `73.62% <ø> (ø)` | | | [gnovm](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `67.92% <ø> (ø)` | | | [misc/genstd](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `79.72% <ø> (ø)` | | | [tm2](https://app.codecov.io/gh/gnolang/gno/pull/3102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `62.34% <ø> (-0.13%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

r3v4s commented 2 weeks ago

txtar for all 5 events 36b767af681588917cfa51a28d833daea2714732

leohhhn commented 2 weeks ago

@r3v4s

The EIP states 3 event types, none of which are Mint/Burn events; these are all transfer events. Why did you choose this approach?

I have a vague memory we talked about this in the GRC20 package as well; Transfer should be able to cover all needed actions. What's the benefit of having distinct events for Mint/Burn?

r3v4s commented 2 weeks ago

@leohhhn I'm aware of EIP 3 event types, and do remember discussion we talked about GRC20.

And Yes, as you said Transfer does cover all (mint, burn, transfer) but current GRC20 impls unique type, I just followed it.

IMHO, distinct event name sounds more straightforward.

dongwon8247 commented 2 weeks ago

@leohhhn As @notJoon described in #2749, we can reduce the number of RPC calls by retrieving only the specific event data we need (e.g., Mint or Burn). Otherwise, applications have to retrieve all Transfer events and filter out the necessary data.

leohhhn commented 2 weeks ago

@r3v4s @dongwon8247

Sounds good! I remember.

Do you think it would be good to emit the Transfer event as well, to follow the standard? Or should we deviate?

I'm okay doing either if this makes it easier for off-chain services.

r3v4s commented 2 weeks ago

@r3v4s @dongwon8247

Sounds good! I remember.

Do you think it would be good to emit the Transfer event as well, to follow the standard? Or should we deviate?

I'm okay doing either if this makes it easier for off-chain services.

For now, I prefer not to(for 2 reasons).

  1. (AFAIK) std.Emit spends gas
  2. If we're willing to add standard event too, it should be handle in another pr (that also includes grc20 to emit standard event)