Repository navigation
feat: add Fynd subsolver provider (COW-1274) - #68
Conversation
…74-fynd-subsolver
…74-fynd-subsolver
…74-fynd-subsolver
jean-neiverth
left a comment
There was a problem hiding this comment.
Why not create a separate app for fynd subsolver? We already have a subsolver-core package, so reusable components can live there. IMO it would be cleaner for deployment config / logs and to avoid that changes in baseline break fynd subsolver and vice versa. Wdyt?
| async synchronize(now: bigint): Promise<boolean> { | ||
| if (!this.budget.consumeRead()) return false; | ||
| try { | ||
| this.live.clear(); |
There was a problem hiding this comment.
[HIGH] this.live.clear() runs before the await, so if this.byos.proposals() throws, the map is already wiped. On the next poll, every tracked order looks new, which should trigger a wave of redundant re-submissions that burns rate budget and creates duplicate proposals. Collect results into a local next = new Map() and only assign this.live = next after the await succeeds.
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^22.0.0", | ||
| "pino-pretty": "^13.1.3", |
There was a problem hiding this comment.
[HIGH] pino-pretty is in devDependencies but the logger in index.ts hardcodes transport: { target: "pino-pretty" } unconditionally. Any environment that prunes dev deps (e.g. pnpm install --prod) should crash at startup with an unresolvable module error. Move it to dependencies or apply the transport only in non-production environments.
| } | ||
| const candidates: OrderbookOrder[] = []; | ||
| for (const order of orders) { | ||
| const held = await this.holdsLiveProposal(order.uid); |
There was a problem hiding this comment.
[MEDIUM] holdsLiveProposal is awaited serially for every solvable order before any quote is fetched. With many orders on BSC this would exhaust the read budget on per-order verification before reaching submission. The live map is already refreshed by synchronize(); relying on its local state within each cycle would eliminate these round-trips.
| target: order.sellToken, | ||
| value: 0n, | ||
| callData: | ||
| `0x095ea7b3${router.slice(2).padStart(64, "0")}${order.remainingSell.toString(16).padStart(64, "0")}` as Hex, |
There was a problem hiding this comment.
[MEDIUM] The ERC-20 approve calldata is assembled by hand (0x095ea7b3...). This would be fragile if the encoding ever changes and bypasses the encodeFunctionData from viem that is already used elsewhere in the codebase. Consider using encodeFunctionData with an inline ERC-20 ABI instead.
| proposalRefreshIntervalSeconds: result.data.PROPOSAL_REFRESH_INTERVAL_SECONDS, | ||
| toml: { | ||
| chainId, | ||
| trampolineFactory: (raw["trampoline-factory"] ?? raw.trampolineFactory) as Address, |
There was a problem hiding this comment.
[MEDIUM] If trampoline-factory is absent from the TOML, this becomes undefined as Address and would surface as a confusing crash inside readContract rather than a clear startup error. The chainId field gets an explicit check; trampolineFactory and the numeric fields (proposalTtl, pollInterval) would benefit from the same treatment — an isAddress guard and positive-integer assertions right here, failing fast with a descriptive message.
| } | ||
|
|
||
| async ready(): Promise<boolean> { | ||
| await this.initialize(); |
There was a problem hiding this comment.
[MEDIUM] ready() calls initialize() on every poll cycle, which re-checks chainId !== 56 and re-fetches info if missing. After the first successful init, this is a no-op, but if initialize() fails during a health check, a transient network error and a permanent config mismatch look the same to the caller. Consider setting a flag after init completes and skipping the body of initialize() entirely on subsequent calls — ready() would then only call client.health().
There was a problem hiding this comment.
The final implementation already caches the validated /info result after startup. Subsequent ready() calls only perform the health check; the repeated chain check is in-memory. I’ll leave this unchanged.
| } | ||
| } | ||
|
|
||
| class FyndSubsolver { |
There was a problem hiding this comment.
[MEDIUM] FyndSubsolver — the most complex class in this PR — has no direct test coverage. Key behaviors that would go undetected on regression: synchronize() error while live is populated (the wipe-before-fetch bug), pollOnce skipping when rate-limited, proposal refresh after the interval elapses, and budget exhaustion during holdsLiveProposal.
…ubsolver # Conflicts: # apps/byos/drizzle/meta/_journal.json # apps/byos/openapi.yml # apps/byos/src/infra/__tests__/storage.db.test.ts # apps/byos/src/infra/api/routes.ts # apps/byos/src/infra/blockchain/operator.ts # apps/byos/src/infra/jobs/validation.ts # apps/byos/src/infra/storage.ts # docs/adr/0013-proposal-lifecycle-and-retention.md
- Remove uniswap fields (fynd-subsolver uses Fynd API, not AMM routing) - Add Dockerfile COPY to include config/bsc/subsolver.toml in runtime image - Set CONFIG_PATH=config/bsc/subsolver.toml when running the fynd-subsolver
feat(subsolver): add BSC subsolver config
Conflicts resolved: - apps/baseline-subsolver/src/index.ts: kept feature branch (randomNonce, lastSubmitted) - apps/baseline-subsolver/src/infra/blockchain.ts: kept feature branch (null-guard over biome-ignore) - pnpm-lock.yaml: regenerated via pnpm install
Summary
Verification
pnpm testpnpm test:dbpnpm test:integrationpnpm typecheckpnpm buildpnpm test:dockerStacked on #67 and bleu/byos-docs#9.
Closes COW-1274.