-
Notifications
You must be signed in to change notification settings - Fork 51
Handles Failed callbacks in process and stores failed statuses #506
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
Handles Failed callbacks in process and stores failed statuses #506
Conversation
joshuahannan
commented
Aug 7, 2025
- Marks processed callbacks that were not succeed as failed
- Stores failed statuses for callbacks in the historic callback field
…o josh/failed-callbacks
contracts/FlowCallbackScheduler.cdc
Outdated
@@ -244,7 +248,7 @@ access(all) contract FlowCallbackScheduler { | |||
access(all) var refundMultiplier: UFix64 | |||
|
|||
/// historic status limit is the maximum age of a historic canceled callback status we keep before getting pruned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add in comment what is the unit (timestamp second)?
contracts/FlowCallbackScheduler.cdc
Outdated
@@ -401,7 +426,7 @@ access(all) contract FlowCallbackScheduler { | |||
Priority.Low: 2.0 | |||
}, | |||
refundMultiplier: 0.5, | |||
historicStatusLimit: 30.0 * 24.0 * 60.0 * 60.0 // 30 days | |||
historicStatusAgeLimit: 30.0 * 24.0 * 60.0 * 60.0 // 5 days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incorrect comment, let's keep it at 30 for now
contracts/FlowCallbackScheduler.cdc
Outdated
if let historic = self.historicCallbacks[id] { | ||
return historic.status | ||
} else if id > self.earliestHistoricID { | ||
return Status.Succeeded | ||
} | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I wonder if explicit Unknown
status would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good suggestion. I'll add Unknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor feedback