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

Fix Reputation Mining Cycle Handler Timestamp #296

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rdig
Copy link
Member

@rdig rdig commented Nov 22, 2024

This PR fixes the timestamp value for the reputationMiningCycle handler, since if the ingestor falls behind the chain, by the time it catches back up to this event it will have used the wrong timestamp for reputation, meaning both updates and UI reporting will be wrong.

This value is mostly used for UI display purposes, however, even so, if it gets out of date, users might get the wrong impression when looking up the reputation cycle's next update in the user hub

@rdig rdig added the bug Something isn't working label Nov 22, 2024
@rdig rdig self-assigned this Nov 22, 2024
@rdig rdig force-pushed the fix/correct-reputation-cycle-timestamp branch from 4c90671 to f586af3 Compare December 2, 2024 17:19
@rdig rdig marked this pull request as ready for review December 3, 2024 16:47
@rdig rdig requested a review from a team December 3, 2024 16:47
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Reviewed alongside the CDapp counterpart.

Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

Tested alongside colonyCDapp#3756 👍

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Damn this was definitely a digging session, props for this solution 💪
Before applying the patch:
image
After:
image

nice one 😎

@rdig
Copy link
Member Author

rdig commented Dec 5, 2024

@bassgeta can I get the "corresponding" review on the CDapp side too please :)

@rdig
Copy link
Member Author

rdig commented Dec 5, 2024

This is on hold until the may deployment goes out, will merge after

@rdig rdig added the on-hold label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working on-hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants