Add core TypeScript submodules: cron, retry, rate-limit, and crypto - #5903
bradleyshep wants to merge 9 commits into
Conversation
clockwork-tien
left a comment
There was a problem hiding this comment.
There are places where we are using snake_case instead of camelCase, worth checking thoroughly to ensure consistency. I have also left additional comments inline.
| "name": "@spacetimedb/crypto", | ||
| "description": "Deterministic hashing, encoding, and webhook-signature helpers for SpacetimeDB TypeScript modules.", | ||
| "version": "0.1.0", | ||
| "license": "BUSL-1.1", |
There was a problem hiding this comment.
Is BUSL-1.1 license intentional?
| : [] | ||
| ); | ||
|
|
||
| export const start_reactor = spacetimedb.procedure( |
There was a problem hiding this comment.
These should be lowerCamelCase
| export const start_reactor = spacetimedb.procedure( | |
| export const startReactor = spacetimedb.procedure( |
| } | ||
| ); | ||
|
|
||
| export const tap_reactor = spacetimedb.procedure( |
There was a problem hiding this comment.
These should be lowerCamelCase
| export const tap_reactor = spacetimedb.procedure( | |
| export const tapReactor = spacetimedb.procedure( |
| } | ||
| ); | ||
|
|
||
| export const buy_upgrade = spacetimedb.procedure( |
There was a problem hiding this comment.
| export const buy_upgrade = spacetimedb.procedure( | |
| export const buyUpgrade = spacetimedb.procedure( |
| } | ||
| ); | ||
|
|
||
| export const repair_reactor = spacetimedb.procedure( |
There was a problem hiding this comment.
| export const repair_reactor = spacetimedb.procedure( | |
| export const repairReactor = spacetimedb.procedure( |
| return isAdmin(ctx) ? takeRows(retryCtx.db.retryTask.iter()) : []; | ||
| } | ||
|
|
||
| function retryHistoryAdmin(ctx: unknown): RetryHistoryRow[] { |
There was a problem hiding this comment.
retryHistory is never pruned, and with takeRows(iter(), 1000) after 1000 attempts the admin view stops showing anything recent
| npm install @spacetimedb/retry spacetimedb@^2.8.3 | ||
| ``` | ||
|
|
||
| Requires SpacetimeDB 2.8.3 or later for submodule mounting. |
There was a problem hiding this comment.
Seems incorrect since nothing here mounts (no export default schema etc)
|
|
||
| ## Requirements | ||
|
|
||
| - SpacetimeDB CLI 2.8.3 |
There was a problem hiding this comment.
Is it expected to pin the requirement to 2.8.3? e.g. the peer dependency is workspace:^
| @@ -0,0 +1,340 @@ | |||
| // Verify the pure-TS implementations against published test vectors. | |||
There was a problem hiding this comment.
Worth adding here that hashing is @noble/hashes, not an implementation in this package
| ranAt: retryCtx.timestamp, | ||
| }); | ||
|
|
||
| const result = dispatchRetry( |
There was a problem hiding this comment.
Seems if a handler throws instead of returning retryFailed, the whole reducer aborts. Worth adding try/catch here
clockwork-tien
left a comment
There was a problem hiding this comment.
I noticed a lot of the public surface is only public because the settings are passed in on every call instead of being encapsulated by the library. I would be interested to hear if there is reasoning behind this.
The example writes its own wrapper in consumer code to call the rate limiter which should be handled internally by the library. Here buildRateLimitKey is exported only so a caller can build the key, and scope is passed in twice. With the current design the caller has to build the argument, so the builder has to be exported.
function consumeAction(
tx: Tx,
scope: string,
actorKey: string,
limit: number,
windowSeconds: number,
cost = 1,
) {
return rateLimit.consumeRateLimit(tx.as.rateLimit, {
key: rateLimit.buildRateLimitKey(scope, actorKey),
scope,
limit,
windowSeconds,
cost,
});
}
const result = consumeAction(tx, TAP_SCOPE, key, tapLimit, TAP_WINDOW_SECONDS);Possible improvements, one way is a client(config) per package:
const tapLimiter = rateLimit.client({ scope: TAP_SCOPE, windowSeconds: TAP_WINDOW_SECONDS });
const result = tapLimiter.consume(tx.as.rateLimit, { key, limit: tapLimit });A few more points:
- Worth exporting the error codes, seems there are quite a few of them, e.g.
rate_limit.not_authorized, since renaming one breaks callers silently - Worth having a setup function per package with consistent naming and signature,
client(config)when the module loads andinstall(ctx)ininit(resend.installrather thanresend.installResend), instead of
spacetimeCron(sdk) + createCron(jobs, opts) // needs both
createRetrySubmodule(deps, handlers, auth?)
installRateLimit(ctx)
installRateLimitState(ctx, opts?)
installResend(ctx)
installRetry(ctx) | export { installRateLimit } from './submodule/install'; | ||
| export { | ||
| DEFAULT_SWEEP_BATCH, | ||
| MAX_SWEEP_BATCH, |
There was a problem hiding this comment.
DEFAULT_SWEEP_BATCH and MAX_SWEEP_BATCH are public only because install.ts and operations.ts import them from '../index' rather than '../limit'
| export { installRateLimit } from './submodule/install'; | ||
| export { | ||
| DEFAULT_SWEEP_BATCH, | ||
| MAX_SWEEP_BATCH, |
There was a problem hiding this comment.
DEFAULT_SWEEP_BATCH and MAX_SWEEP_BATCH are public only because install.ts and operations.ts import them from '../index' rather than '../limit'
| DEFAULT_SWEEP_BATCH, | ||
| MAX_SWEEP_BATCH, | ||
| assertRateLimitSweepBatch, | ||
| DEFAULT_SWEEP_INTERVAL_SECONDS, |
There was a problem hiding this comment.
DEFAULT_SWEEP_INTERVAL_SECONDS is not used outside limit.ts
| export interface ConsumeRateLimitOpts { | ||
| key: string; | ||
| scope: string; | ||
| limit: number; | ||
| windowSeconds: number; | ||
| cost?: number; | ||
| } |
There was a problem hiding this comment.
The policy is passed on every call so 2 places can set the same limit differently and a typo in scope makes a new bucket
| let deleted = 0; | ||
| for (const row of expiredRows) { | ||
| if (deleted >= maxRows) break; | ||
| if ((row.expiresAt.microsSinceUnixEpoch as bigint) > nowMicros) break; |
There was a problem hiding this comment.
The sweep breaks on the first unexpired row so with iter() everything past it is skipped
| * contains at least one valid v1 signature AND the timestamp is within | ||
| * tolerance. | ||
| */ | ||
| export function verifyStripeSignature(opts: StripeVerifyOpts): boolean { |
There was a problem hiding this comment.
All failures return false so failure info is swallowed. Worth returning { ok: false, reason } instead, since it is a library and the consumer decides what to surface
| { name: 'rate_limit_admin_identity', public: false }, | ||
| { | ||
| identity: t.identity().primaryKey(), | ||
| addedAtMicros: t.i64(), |
There was a problem hiding this comment.
Here is t.i64() where every other time column is t.timestamp() so clients get a plain number for this one
|
|
||
| function requireAdmin(ctx: ReducerModuleCtx): void { | ||
| if (ctx.db.rateLimitAdminIdentity.identity.find(ctx.sender) == null) { | ||
| throw new SenderError('rate_limit.not_authorized'); |
There was a problem hiding this comment.
This string is the entire response body, with no code field beside it, and it is not exported. The only way to branch on the reason is to compare the string, so a rename breaks consumers with no warning
| @@ -0,0 +1,17 @@ | |||
| export { sha256, SHA256_BYTES } from './sha256.ts'; | |||
There was a problem hiding this comment.
Importing with .ts needs allowImportingTsExtensions, and the tsconfig spacetime init generates does not set this so the first spacetime build fails. Worth shipping compiled dist instead
| const out = new Uint8Array((s.length / 4) * 3 - pad); | ||
| let oi = 0; | ||
| for (let i = 0; i < s.length; i += 4) { | ||
| const c0 = lookup[s.charCodeAt(i)]; |
There was a problem hiding this comment.
Here base64 accepts invalid input so a bad signing secret fails silently. With lookup 256 long, a char with a code above 255 is undefined, undefined < 0 is false so it decodes as A instead of hitting the base64: invalid char guard just below
Configure fixed policies once, expose stable errors, keep installers idempotent, and support admin revocation. Remove internal sweep exports and align init naming. Verify packed consumers with the default TypeScript template.
Description of Changes
Adds reusable scheduling, retry, rate-limit, and cryptographic helpers for SpacetimeDB TypeScript modules.
This is the prerequisite branch for the remaining submodule groups. It is based on master commit
3653d2ed4.Example screenshots
Existing example screenshots from #5823:
Cron
Rate Limit
API and ABI breaking changes
No existing SpacetimeDB API or ABI is changed. This adds new TypeScript package APIs that need review before merge.
Rollback safety impact
n/a. This adds opt-in TypeScript packages and examples; it does not change existing server storage formats.
Expected complexity level and risk
3/5
The packages are opt-in and do not change existing server behavior. Complexity is in scheduling, retry recovery, and cron's direct use of the internal
spacetime:sys@2.0host ABI. Review failure recovery and repeated execution for applications that use these packages.Testing
Verified locally on
bradley/submodules-core:Commands used for this group:
The TypeScript SDK was built first. Live deployment and provider tests were not run during split validation. Existing workspace peer-dependency warnings remain.