Addresses Josh and Janez PR comments #510
Conversation
joshuahannan
changed the base branch from
master
to
feature/callback-scheduling
contracts/FlowCallbackScheduler.cdc
Outdated
| @@ -402,7 +403,7 @@ access(all) contract FlowCallbackScheduler { | |||
|
|
|||
| /// Check if there are any timestamps that need processing | |||
| /// Returns true if processing is needed, false for early exit | |||
| access(all) fun check(current: UFix64): Bool { | |||
| access(all) fun checkIfTimestampsNeedProcessing(current: UFix64): Bool { | |||
There was a problem hiding this comment.
More consistent naming if we want to have it verbose style
| access(all) fun checkIfTimestampsNeedProcessing(current: UFix64): Bool { | |
| access(all) fun hasBefore(current: UFix64): Bool { |
There was a problem hiding this comment.
good suggestion. That is a much better name
contracts/FlowCallbackScheduler.cdc
Outdated
| @@ -388,7 +389,7 @@ access(all) contract FlowCallbackScheduler { | |||
| } | |||
|
|
|||
| /// Get all timestamps that are in the past (less than or equal to current timestamp) | |||
| access(all) fun past(current: UFix64): [UFix64] { | |||
| access(all) fun getTimestampsBefore(current: UFix64): [UFix64] { | |||
There was a problem hiding this comment.
I actually preffer having more concise names, and if something is on the type it doesn't have to repeat the type, so it's getBefore becasue you would anyway have it called like: pastTimestamps.getBefore() while pastTimestamps.getTimestampsBefore() just repeats the type. This is subjective tho so don't want to force my opinion.
There was a problem hiding this comment.
I like getBefore. I'll update that
contracts/FlowCallbackScheduler.cdc
Outdated
| @@ -905,7 +910,7 @@ access(all) contract FlowCallbackScheduler { | |||
| callbackOwner: callback.handler.address | |||
| ) | |||
| } else { | |||
| panic("Invalid Status: \(id) wrong status \(callback.status.rawValue)") // critical bug | |||
| panic("Invalid Status: Callback with id \(id) has wrong status \(callback.status.rawValue)") // critical bug | |||
There was a problem hiding this comment.
NIT I think I've changed this to (in another PR, not sure where that is), but it feels better to first say the status after invalid status:... I made it that way so it's same as you made other errors.
| panic("Invalid Status: Callback with id \(id) has wrong status \(callback.status.rawValue)") // critical bug | |
| panic("Invalid Status: \(callback.status.rawValue) for callback id \(id)") // critical bug |
tests/CallbackScheduler_test.cdc
Outdated
|
|
||
| // process the callbacks to make sure the garbage collection is triggered | ||
| processCallbacks() | ||
| // processCallbacks() |
There was a problem hiding this comment.
is it ok to leave things commented out like this? why not remove it?
There was a problem hiding this comment.
I should have just deleted it but I forgot. I removed it in the latest commit
Addresses the comments in #508 and #489
processExecuteentitlement