allow timestamp overrides #414
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
examples/with-ethers/src/index.ts
Outdated
| // ----- | ||
| // | ||
| // Optional flag to override timestamp using Turnkey's system clock | ||
| // overrideTimestamp: true, |
There was a problem hiding this comment.
This name is a bit odd, I was expecting to see a way for people to pass their own timestamps (idea of an "override"). Instead it seems like what is really happening is we give people the option to use a remote server's time. So maybe let's name this "useTurnkeyRemoteTimestamp"?
packages/http/src/shared.ts
Outdated
| export async function getLiveTimestamp(): Promise<string> { | ||
| const timestampResponse = await fetch("https://api.turnkey.com/health"); | ||
| const parsedResponse = await timestampResponse.json(); | ||
|
|
||
| return parsedResponse.currentTime ?? ""; | ||
| } |
There was a problem hiding this comment.
This might be okay but I wonder how much extra traffic that's going to cause. This code will live "forever" so I'm a bit nervous about the lack of caching.
An idea that we could implement potentially would be a one-time fetch to "set the clock". Then the local timestamps can still be used as a relative source of time.
- SDK init: fetch api.turnkey.com/health, get timestamp
- compare remote with local time, store delta
- when requests need a timestamp, get local time + delta
That's under the assumption that the user clocks are off by an hour or a day or a year...basically a fixed offset. Basically I'm assuming that their local device can still increment time correctly.
If we still see problems with this logic we can periodically "resync" clocks (refresh the delta between remote and local)
This solution is kind of nice because we start with a single init-time call which calls health: this has the benefit of "syncing" time, but also ensure that our API isn't down. If /health fails it's good to know ahead of time before making any API request (perfect place to insert telemetry reporting btw!)
packages/http/src/shared.ts
Outdated
| * @returns string reprenting timestamp in seconds | ||
| */ | ||
| export async function getLiveTimestamp(): Promise<string> { | ||
| const timestampResponse = await fetch("https://api.turnkey.com/health"); |
There was a problem hiding this comment.
I think we should make the hostname overridable or configurable? If I set my turnkey API host to preprod, this would also use preprod health.
andrewkmin
force-pushed
the
andrew/override-timestamp
branch
from
81ae27e to
b8fdceb
Compare
Summary & Motivation
$title
The main use case this serves is when a client may have an incorrect system time, leading to rejections by our servers. See https://docs.turnkey.com/faq#how-long-is-a-signed-activity-request-valid-for for more details.
See https://github.com/tkhq/mono/pull/3444 for mono counterpart.
How I Tested These Changes
Examples
Did you add a changeset?
Yes 🖖
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.