Break process logic into multiple steps #514
Conversation
| // Add the execution effort for this callback to the total for the slot's priority | ||
| let slotEfforts = self.slotUsedEffort[slot]! | ||
| slotEfforts[callback.priority] = slotEfforts[callback.priority]! + callback.executionEffort | ||
| self.slotUsedEffort[slot] = slotEfforts |
There was a problem hiding this comment.
Why no check if the priority isn't Low? We can't really add to the used effort for a low priority in a slot because that might get taken up by a medium or high priority callback later
There was a problem hiding this comment.
yeah we should have the if check, this is a copy-paste mistake (sorry was in a rush a bit)
| if callback.executionEffort <= lowEffortLeft { | ||
| low.append(callback) | ||
| } | ||
| } | ||
|
|
||
| emit PendingExecution( | ||
| id: id, | ||
| priority: callback.priority.rawValue, | ||
| executionEffort: callback.executionEffort, | ||
| fees: callback.fees.balance, | ||
| callbackOwner: callback.handler.address | ||
| ) | ||
|
|
||
| // after pending execution event is emitted we set the callback as executed because we | ||
| // must rely on execution node to actually execute it. Execution of the callback which is | ||
| // done in a separate transaction that calls executeCallback(id) function can not update | ||
| // the status of callback or any other shared state, since that blocks concurrent callback | ||
| // execution. Therefore optimistic update to executed is made here to avoid race condition. | ||
| callback.setStatus(newStatus: Status.Executed) | ||
|
|
||
| // charge the fee for callback execution | ||
| destroy callback.payAndRefundFees(refundMultiplier: 0.0) | ||
| lowEffortLeft = lowEffortLeft - callback.executionEffort |
There was a problem hiding this comment.
Shouldn't the subtraction be within the if statement? We only want to subtract it from the effort left if we have added it to the pending callbacks list
| destroy callback.payAndRefundFees(refundMultiplier: 0.0) | ||
| } | ||
|
|
||
| self.removeExecutedCallbacks() |
There was a problem hiding this comment.
This can't be done here. Remember, we can't remove callbacks until after they are actually executed because that also removes the callback resource, meaning there would be no callback to execute, so they can't be removed in the same transaction that they are processed. The easiest change here would be to just put the call to removeExecutedCallbacks before the call to pendingQueue`
There was a problem hiding this comment.
you are right, again a bit in a rush and unfinished state. Let's not put it in the pendingQueue tho because that's will change the purpose of that function, let's just put it at the begining of the function after the if length != 0 check
joshuahannan
merged commit 87664b5
into
onflow:josh/address-audit-feedback
Improve separation of concerns and testability by extracting queue processing logic into dedicated functions.
Key Changes
pendingQueue()function: Isolates the logic for creating a queue of callbacks ready for execution. This function takes current blockchain state and returns an ordered list of pending callbacks without side effects.removeExecutedCallbacks()function: Moves cleanup logic for executed callbacks into its own function, making the codebase more modular and easier to test.process()function: The main processing function now delegates to the new functions, making the control flow clearer and more maintainable.garbageCollect()toremoveCallback(): More descriptive function name that better reflects its purpose.Benefits