-
Notifications
You must be signed in to change notification settings - Fork 15
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
Migrated codebase to typescript #33
Conversation
CluEleSsUK
commented
Jul 7, 2022
•
edited
Loading
edited
- modified directory structure to separate tests and lib
- migrated codebase to Typescript
- replaced standard linter with eslint
- replaced ava with jest + ts-jest
- fixed a few small bugs along the way
- fixed optimiser tests and stub - fixed a few setTimeout leaks
fixed some broken and incorrect tests
no new features or any API changes per se, but typescript upgrade seemed more than a patch from a risk POV
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.
A few comments. LGTM
}> | ||
|
||
export interface NetworkClient { | ||
get(round?: number, options?: ClientOptions): Promise<RandomnessBeacon> |
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.
Should we have the chainhash as an optional param here rather than in the DrandOptions?
this.controllers.push(controller) | ||
|
||
try { | ||
const url = `${this.url}/public/${round || 'latest'}${options.noCache ? '?' + Date.now() : ''}` |
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.
are you handling the chain hash at the url level then?
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.
I just translated the old impl and curiously it ignores the chainHash
and just calls the default public
, e.g. https://pl-us.testnet.drand.sh/public/latest
.
This needs rethought for sure
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.
That's because the chainHash endpoint is a new thing brought by the multibeacon support in 2022.
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.
ahhhh right, that makes sense then. In that case, I will raise a separate PR for that when I implement unchained beacon verification (which I have already basically done in timelock, so I will port it over)
(as per the go client)
coerced some ms to s for a DateTime