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

add reverted field to database and expose telemetrics for reverted events #97

Merged
merged 14 commits into from
Feb 20, 2025

Conversation

NikolasHaimerl
Copy link
Contributor

This PR proposes the following changes:

  1. Add the reverted field to the table of active_deals
  2. Count the number of reverted active deals and add them to InfluxDB

The implementation plan for this PR can be found here.
Closes #22

@NikolasHaimerl NikolasHaimerl marked this pull request as ready for review February 12, 2025 10:37
@juliangruber

This comment was marked as resolved.

@NikolasHaimerl NikolasHaimerl changed the title add reverted field to database add reverted field to database and expose telemetrics for reverted events Feb 13, 2025
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

On second thought, I don't think a deal can be reverted. It's only an event that's reverted. While I am not entirely familiar with how this works on the Filecoin side, I would expect a sequence like this:

  • Event 1 is emitted, uses e.g. sector: 123
  • Chain re-org happens, deal fields change
  • Event 1 is reverted
  • Event 2 is emitted. Most of the fields from Event 1 are preserved, but some are changed, e.g. sector: 125.

To correctly handle that sequence, we need to remove a deal from active_deals when an event was reverted, so that it can be added again in the future.

Or maybe we can treat reverted: true as a soft-delete flag? But then the loops looking up payload CID and uploading deals to spark-api must be updated accordingly.

I think it would be best to not make assumptions and either find documentation of ask FilOz team to better understand what's the correct way to process reverted claim events.

@NikolasHaimerl
Copy link
Contributor Author

On second thought, I don't think a deal can be reverted. It's only an event that's reverted. While I am not entirely familiar with how this works on the Filecoin side, I would expect a sequence like this:

* Event 1 is emitted, uses e.g. `sector: 123`

* Chain re-org happens, deal fields change

* Event 1 is reverted

* Event 2 is emitted. Most of the fields from Event 1 are preserved, but some are changed, e.g. `sector: 125`.

To correctly handle that sequence, we need to remove a deal from active_deals when an event was reverted, so that it can be added again in the future.

Or maybe we can treat reverted: true as a soft-delete flag? But then the loops looking up payload CID and uploading deals to spark-api must be updated accordingly.

I think it would be best to not make assumptions and either find documentation of ask FilOz team to better understand what's the correct way to process reverted claim events.

As this PR only focuses on the observability of when events with a reverted filed set to true occur, does your suggestion influence this PR?

juliangruber

This comment was marked as resolved.

juliangruber

This comment was marked as resolved.

@NikolasHaimerl

This comment was marked as resolved.

pyropy

This comment was marked as resolved.

@NikolasHaimerl

This comment was marked as resolved.

@NikolasHaimerl NikolasHaimerl merged commit 3205201 into main Feb 20, 2025
8 checks passed
@NikolasHaimerl NikolasHaimerl deleted the nhaimerl-reverted-deals branch February 20, 2025 09:24
Copy link

sentry-io bot commented Feb 20, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Connection terminated unexpectedly countRevertedActiveDeals(resolve-payload-cids) View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle reverted events
4 participants