FEATURE: Callback scheduling #489
Conversation
| /// calculate fee by converting execution effort to a fee in Flow tokens. | ||
| access(all) fun calculateFee(executionEffort: UInt64, priority: Priority, data: AnyStruct?): UFix64 { | ||
| // Use the official FlowFees calculation | ||
| let baseFee = FlowFees.computeFees(inclusionEffort: 1.0, executionEffort: UFix64(executionEffort)) |
There was a problem hiding this comment.
This links to my comment here onflow/flips#331 (comment).
You are adding inclusion fees here, so you do not need to have a minimum execution effort limit. I would have the inclusion effort configurable though.
There was a problem hiding this comment.
I don't think it makes sense to have a configurable inclusion fee because that would require us to predict network load for the future, which isn't possible, right? We'd have to predict it for each timestamp. Additionally, the fee multipliers are already configurable, so that kind of solves the same problem as a configurable inclusion fee. I also responded to your comment on the FLIP about the minimum effort
| originalTimestamp: sanitizedTimestamp, | ||
| priority: priority, | ||
| executionEffort: executionEffort, | ||
| fees: <- fees, |
There was a problem hiding this comment.
the fees include the storageFee but the since its still in the CallbackData it doesn't actually help the account holding this data with storage capacity. Is this ok? do we just plan to have a high enough balance on the account for this to not be a problem?
There was a problem hiding this comment.
that is a good point. I think that we'll have enough FLOW in the account that we won't have to worry about it, but we still should probably deposit those extra storage fees to the account's flow vault.
Now that I am thinking about it, I kind of want to just deposit all fees to the account's vault temporarily instead of storing them in CallbackData. It simplifies the contract but it does create a bigger honeypot that would be worse if a vulnerability was found. How does that sound?
There was a problem hiding this comment.
I don't think this is a big concern, probably we ran out of computation before storage.
There was a problem hiding this comment.
@bluesign what do you mean? like the computation of storing a large piece of data is more costly than the actual storage fees?
There was a problem hiding this comment.
yeah I think adding ( writing ) some bytes ( considering it is small ) to a large dictionary ( where we may have hypothetical problem ) will be more cheaper in storage fees vs computation required.
It is pretty hard to predict as we don't have fees technically, and luckily no-one is abusing.
|
|
||
| // if there is no space left for medium priority we search for next available timestamp | ||
| // todo: check how big the callstack can grow and if we should avoid recursion | ||
| return self.calculateScheduledTimestamp( |
There was a problem hiding this comment.
can we try doing this without recursion? Recursion is costly.
There was a problem hiding this comment.
How costly is recursion? I don't see this ever getting to more than a few levels deep and this is a clean way to do it
There was a problem hiding this comment.
I think stack depth is 2000, cost to authorizer is negligible ( though to system it can have heavier cost )
* Optimise processing with sorted timestamps * Add tests for sorted timestamps type * Fix wrong assert args * Extract garbage collection * Add missing arg * Handle wrong statuses * Change access control to account * Change access control to only limit process and execute to FVM * Add comments * Regenerate all the assets * generate assets --------- Co-authored-by: Joshua Hannan <[email protected]>
|
Thanks for the feedback @janezpodhostnik! I opened a PR addressing some of your comments and left questions and comments on the other ones |
| let type = data!.getType() | ||
| if type.isSubtype(of: Type<Number>()) | ||
| || type.isSubtype(of: Type<Bool>()) | ||
| || type.isSubtype(of: Type<Path>()) |
There was a problem hiding this comment.
The size of Path is not any more bounded than e.g. a String so seems counterintuitive to round it to 0.0 regardless of length.
See e.g.:
transaction {
execute {
var stringToStore = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
log("Length of string: ".concat(stringToStore.length.toString()))
log(
"getSizeOfData if stored as string:".concat(
FlowCallbackScheduler.getSizeOfData(
stringToStore
).toString()
)
)
log(
"getSizeOfData if masked as path:".concat(
FlowCallbackScheduler.getSizeOfData(
StoragePath(identifier: stringToStore)
).toString()
)
)
}
}
Produces:
6:51AM INF LOG: "Length of string: 510"
6:51AM INF LOG: "getSizeOfData if stored as string:0.00057500"
6:51AM INF LOG: "getSizeOfData if masked as path:0.00000000"
There was a problem hiding this comment.
good point. I didn't consider that! I'll remove Path from the list
| if callbackEffort <= lowPriorityEffortAvailable { | ||
| lowPriorityEffortAvailable = lowPriorityEffortAvailable - callbackEffort | ||
| lowPriorityCallbacks.remove(key: lowCallbackID) | ||
| sortedCallbackIDs.append(lowCallbackID) |
There was a problem hiding this comment.
The FLIP says that a low priority callback "is executed opportunistically, only in blocks after the scheduled timestamp".
However, it seems this code may process low-priority callbacks before they are due - lowPriorityCallbacks is a collection of all low-priority callbacks as they are all stored at the special timeslot 0.
There was a problem hiding this comment.
oh man, I can't believe I didn't have a test for that. Good catch!
| // Remove fractional values from the timestamp | ||
| let sanitizedTimestamp = UFix64(UInt64(timestamp)) | ||
|
|
||
| if sanitizedTimestamp <= getCurrentBlock().timestamp { |
There was a problem hiding this comment.
The FLIP says about scheduling inputs
"The special timestamp value "0" can be used to mean "as soon as possible"
However, this functionality is not implemented by the contract and setting timestamp: 0 will fail this check. Maybe worth adding at least a TODO? When this is implemented, we should be careful not to conflict with logic for low-priority callbacks which also uses the special timestamp "0". Alternatively, the above sentence can be removed from the FLIP.
There was a problem hiding this comment.
We actually are removing the special low priority timestamp from the contract. and I think we should remove the as soon as possible timestamp from the FLIP too. @devbugging what do you think?
| Priority.Medium: mediumPriorityEffortReserve + sharedEffortLimit, | ||
| Priority.Low: 5_000 | ||
| }, | ||
| minimumExecutionEffort: 5, |
There was a problem hiding this comment.
nit: FLIP says "The provided execution effort value must be bigger than 10". Even though this is configurable, would IMO make sense to align the default value and the value mentioned in the FLIP.
There was a problem hiding this comment.
good point. We'll update that
Implements the contracts and tests for the Scheduled Callbacks FLIP