Skip to content
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(ghverify): emit event when user request verification #2778

Merged

Conversation

Villaquiranm
Copy link
Contributor

related to #2777

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
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Sep 9, 2024
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.10%. Comparing base (f6bd2d3) to head (a94ebe3).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2778      +/-   ##
==========================================
+ Coverage   63.08%   63.10%   +0.02%     
==========================================
  Files         563      563              
  Lines       79254    79333      +79     
==========================================
+ Hits        49998    50064      +66     
- Misses      25896    25905       +9     
- Partials     3360     3364       +4     
Flag Coverage Δ
contribs/gnodev 60.62% <ø> (ø)
contribs/gnofaucet 14.82% <ø> (ø)
gno.land 67.56% <ø> (ø)
gnovm 67.25% <ø> (ø)
misc/genstd 79.72% <ø> (ø)
misc/logos 19.45% <ø> (ø)
tm2 62.26% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is necessary. How does this event give more information to an off-chain user, if they could just use the indexer with this query?

query {
  transactions(filter: {
    message: {
      route: vm,
      type_url: add_package,
      vm_param: {
        exec: {
          pkg_path: "gno.land/r/gnoland/ghverify",
          func: "RequestVerification"
        }
      }
  	}
  }) {
    hash
    messages {
      value {
        ... on MsgCall {
          args
        }
      }
    }
    gas_wanted
    success
    response {
      error
    }
  }
}

@leohhhn
Copy link
Contributor

leohhhn commented Sep 12, 2024

@thehowl

We should always practice emitting events, even if we have an indexer.

@n0izn0iz
Copy link
Contributor

n0izn0iz commented Sep 12, 2024

@thehowl if a realm (like a dao or an abstracted account) calls RequestVerification, it will be ignored by agents using your solution

@Kouteki Kouteki added review/triage-pending PRs opened by external contributors that are waiting for the 1st review and removed review/triage-pending PRs opened by external contributors that are waiting for the 1st review labels Oct 3, 2024
@thehowl
Copy link
Member

thehowl commented Oct 23, 2024

After the discussion in #2777, I'm OK with the event being emitted - but I still think we need to use queries as a way to get the source of truth, and use events only as a pub/sub mechanism.

@thehowl thehowl changed the title feat: emit event on ghverify realm when user request verification feat(ghverify): emit event realm when user request verification Oct 23, 2024
@thehowl thehowl changed the title feat(ghverify): emit event realm when user request verification feat(ghverify): emit event when user request verification Oct 23, 2024
@thehowl thehowl merged commit 247f2c6 into gnolang:master Oct 23, 2024
121 of 122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants