-
Notifications
You must be signed in to change notification settings - Fork 212
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
docs: improved TimerService type documentation for auto-gen docs #9131
Conversation
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.
one question
packages/time/src/types.d.ts
Outdated
* follow wall clock time, using the correct types means you don't have to worry | ||
* about timezones or how time is represented on a particular chain. This also |
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.
Did you mean "using the correct brands" rather than "using the correct types"? It's not clear to me how using the correct types means I don't have to worry about the rest. Perhaps it means I'll be sure to get early errors if I do it wrong, but "don't have to worry about it" seems much stronger than that.
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.
You are correct. Getting the types right means you won't be adding 3pm and 5pm, but getting the brands right means you won't be subtracting 20 blocks from 10am.
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.
looks good, a few small suggestions
packages/time/src/types.d.ts
Outdated
@@ -190,6 +211,10 @@ export interface TimerWaker { | |||
wake: (timestamp: TimestampRecord) => void; | |||
} | |||
|
|||
/** | |||
* Provides the ability to schedule wake() calls repeatedly at a regular | |||
* interval, or to disable all future use of this TimerRepeater. |
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, maybe add "created by the old makeRepeater()
API".. which is deprecated (but probably not visibly enough), new code should use repeatAfter()
, which doesn't have a control object and doesn't require a second schedule
step
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.
done
packages/time/src/types.d.ts
Outdated
* The brands prevent you from accidentally combining time values from different | ||
* TimerServices. If some chains track time in blocks, while others | ||
* follow wall clock time, using the correct brands means you don't have to worry | ||
* about timezones or how time is represented on a particular chain. This also |
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, timezones shouldn't be a factor because they are merely a difference in formatting, and of course all sensible code should be using seconds-since-unix-epoch. But this does help manage the difference between consensus time on chain 1, consensus time on chain 2, and the time on your local computer, all of which might be nominally unix time but can diverge in various interesting ways.
Maybe:
The brands prevent you from accidentally combining time values from different
TimerServices. Some chains track time in blocks, others follow wall clock
time, some do both. Every local computer has its own unique notion of wall
clock time. Even when these clocks are talking about the same thing (UTC),
they can all drift in different ways. Using the correct brands lets you be
precise about which particular source of time you mean, preventing confusion
or attacks when the clocks diverge. Thus it is an error to e.g. use a time
you got from chain A to schedule an event on chain B.
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.
done
refs: docs:1024
Description
Improved docs for TimerService types, since it will be used immediately to auto-gen documentation for developers.
Security Considerations
None
Scaling Considerations
None
Documentation Considerations
It's documentation.
Testing Considerations
None
Upgrade Considerations
None