Skip to content

Add grpcToken option for authenticated gRPC endpoints - #99

Merged
11felix merged 3 commits into
mainfrom
chore/grpc-token-option
Aug 15, 2026
Merged

Add grpcToken option for authenticated gRPC endpoints#99
11felix merged 3 commits into
mainfrom
chore/grpc-token-option

Conversation

@11felix

@11felix 11felix commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Adds grpcToken to AlphaFiSDKConfig, StrategyContext (5th param), and BlockchainOptions, attached as x-token gRPC metadata on the SDK's SuiGrpcClient. Needed so alphafi-fe/alphafi-admin can point the SDK's gRPC reads at BlockPI, which authenticates by key (their keys are domain-whitelisted). No behavior change when the option is omitted.

@11felix
11felix requested a review from jangid as a code owner August 12, 2026 13:17
@jangid

jangid commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

operate.md review — head of alphafi-sdk-js#99. First review on this PR.

Small, well-shaped change: grpcToken threaded from AlphaFiSDKConfigStrategyContextBlockchainOptions → the gRPC client's meta as x-token. Checked the things that could go wrong:

  • Fully optional at every layer. grpcToken? on all three types, and meta resolves to undefined when absent, so existing callers that pass no token are unaffected — no behaviour change for anyone not opting in.
  • x-token matches the deployment contract. alphafi-aws#27's guide documents BlockPI auth as x-token gRPC metadata; this agrees, so SDK and infra won't disagree on the header name.
  • The comment about the direct property is the right call and self-verifying. Writing meta: outright rather than a conditional spread means tsc checks the option name against SuiGrpcClientOptions — and build/lint/test are green, which is itself the evidence that meta is a real option on that type.
  • Positional-parameter growth is contained. StrategyContext's constructor is now five positionals, but it's called in exactly one place with all of them named at the call site, so there's no silent-misordering risk today.

Non-blocking notes

  • 🟢 The token now travels inside a plain options object. BlockchainOptions is a bag that could plausibly end up in a log line, an error message, or a cache key. Nothing in this diff does that, so this is a "keep it in mind" rather than a finding — but a credential inside an options object is one JSON.stringify(options) away from a log leak. If this repo ever adds debug logging of construction, grpcToken is the field to redact.
  • 🟢 AlphalendClient is still constructed with (network, graphqlUrl) and gets no token. Correct for this PR's scope — flagging only so it's a deliberate omission rather than a forgotten one, since the alphalend side has its own credential path.
  • 🟢 A sixth positional would be the point to switch StrategyContext to an options object.

Sequencing (the reason this one matters first)

alphafi-admin#34 currently fails typecheck with TS2353: 'grpcToken' does not exist in type 'AlphaFiSDKConfig' and TS2554: Expected 1-4 arguments, but got 5 — i.e. it is written against exactly the surface this PR adds. Both consumers take the SDK as a file:../ path dependency, so their CI resolves the sibling at main and stays red until this merges. This PR is the unblocker for the rest of the BlockPI migration and should merge first.

CI green (build / lint / test). Not a contract PR. No open threads. Approving.

jangid
jangid previously approved these changes Aug 12, 2026

@jangid jangid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operate.md: clean — CI green (build/lint/test), change is additive and optional at every layer, x-token agrees with the deployment contract in alphafi-aws#27. Non-blocking note only: the token now lives inside a plain options object, so redact grpcToken if construction ever gets debug logging. Sequencing: this is the unblocker — alphafi-admin#34 fails typecheck specifically on the surface this PR adds, so it should merge ahead of the two frontends. Approvals: 1/2 — needs 1 more before merge.

@11felix
11felix requested a review from Zorag44 August 13, 2026 12:50
@Zorag44

Zorag44 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

operate.md review — head b5c9ae5. Follow-up pass; my earlier review of 2b6db41 stays above unedited for the record, and the blocking finding it raised is now closed.

✅ Blocking finding resolved

Re-derived against the current diff rather than taken on the reply:

  • Explicit transportsrc/models/blockchain.ts:43-56. meta now rides on a GrpcWebFetchTransport the code constructs itself, and baseUrl moved onto the transport, which is what the { transport } | SuiGrpcTransportOptions union requires.
  • Dependency declaredpackage.json:49 adds @protobuf-ts/grpcweb-transport at ^2.11.1, the same range @mysten/sui declares for it. package-lock.json resolves a single hoisted copy at 2.11.1, so there is no second transport implementation in the tree.
  • The property is under test, and the test actually runssrc/__tests__/blockchain-grpc-transport.test.ts asserts defaultOptions.meta equals { 'x-token': … } with a token and is undefined without. I checked it's matched by testMatch: ['**/src/__tests__/**/*.test.ts'] rather than assuming, because passWithNoTests: true would let a misplaced file pass silently — and confirmed in the CI log for this head: PASS src/__tests__/blockchain-grpc-transport.test.ts, Tests: 2 passed, 2 total.
  • Empty-string tokens send no header — the conditional spread is falsy on '', so a blank env var can't produce an empty x-token. Matches the guard alphafi-admin#34 called out.

The live A/B against BlockPI (Apikey not found → authenticated read) also closes the one thing I'd flagged as unverified on my side: I'd only shown the header was absent, not what BlockPI does with an anonymous gRPC request. That's now settled empirically, and it's the acceptance check this change needed.

🟢 Non-blocking — not reasons to withhold approval

  • The devnet narrowing is still here, and now inconsistent with the sibling fix. src/models/blockchain.ts:41 and :48 both keep options.network === 'testnet' ? 'testnet' : 'mainnet'. Pre-existing and untouched by this PR, so out of scope — but Network from @alphafi/alphalend-sdk includes devnet, and alphalend-sdk-js#176 just fixed the identical line and documented why (mislabelling confuses network-keyed resolution such as MVR). The two SDKs now disagree on the same question. Cheap follow-up.
  • The other new Blockchain(...) sites build an unused gRPC client, and that is safe — I checked rather than assumed. Blockchain's own methods are GraphQL throughout (multiGetReceipts is a GraphQL query at :316; suiGrpcClient appears only as the field and its assignment), so the untokened instance at src/services/cetusCompensation.ts:119 makes no gRPC calls. Every live gRPC consumer — src/admin/alphaVault.ts:59, src/admin/rebalanceCap.ts:16, src/admin/slushAdmin.ts:69,124, and the Navi client in src/strategies/lending.ts:281,951 / src/strategies/looping.ts:298,701 — reaches it through StrategyContext, which threads the token. So no unauthenticated live path remains. Constructing a transport nobody reads is minor waste, not a correctness issue.

CI green (build / lint / test). Not a contract PR. Approving.

Standing note: the earlier approval on this PR was dismissed by the push (the repo's ruleset sets dismiss_stale_reviews_on_push), so this takes the count to 1/2 — one more approval is needed before merge.

Prior findings on this PR are resolved; nothing from either earlier pass remains open.

@11felix

11felix commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@Zorag44 Confirmed and fixed in b5c9ae5. Reproduced your finding exactly before changing anything: read the installed dist/grpc/client.mjs constructor (only baseUrl/fetchInit forwarded), then ran a live A/B against BlockPI with a real key — client-level metaApikey not found, explicit transport → authenticated mainnet data. Also tested the fetchInit-with-headers idea for completeness: dead at the type (headers is in the Omit) and at runtime (the transport's Object.assign order overwrites fetchInit.headers with its own meta-built headers) — empirically identical to sending nothing.

The fix is your suggested shape: explicit GrpcWebFetchTransport with baseUrl + conditional meta, @protobuf-ts/grpcweb-transport declared as a direct dependency pinned to @mysten/sui's own range (^2.11.1), and the test you asked for — src/__tests__/blockchain-grpc-transport.test.ts pins defaultOptions.meta carrying x-token with a token and undefined without. You were also right that green CI was never evidence here; the acceptance check for this change was a live authenticated read through the constructor, which now succeeds.

Same fix landed across the whole scope you flagged: alphalend-sdk-js#176 (+ your devnet-narrowing fix) and #174 (coinHelpers), alphafi-admin#34, alphafi-fe#334 — verified by sweep that no client-level meta: construction remains anywhere in the org's TS code. (alphalend-liquidator's CLI already used the explicit transport with a comment documenting this exact bug.)

@11felix
11felix requested a review from jangid August 14, 2026 07:27

@Zorag44 Zorag44 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operate.md: clean — blocking gRPC metadata finding resolved (explicit transport at src/models/blockchain.ts:43-56, dependency declared, and the new test confirmed running in CI). Non-blocking notes in the summary comment. Approvals: 1/2 — needs 1 more before merge.

@jangid jangid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operate.md: clean — CI green (lint / test / build), head b5c9ae5. Not a contract PR. Zero review threads open.

My earlier approval on this PR was dismissed by the push that fixed the meta-drop finding, so this is a re-approval against the new head, re-derived rather than reinstated. Verified at b5c9ae5: src/models/blockchain.ts builds an explicit GrpcWebFetchTransport with baseUrl on the transport and a conditional meta, @protobuf-ts/grpcweb-transport is declared at ^2.11.1 with a single hoisted lockfile resolution, and src/__tests__/blockchain-grpc-transport.test.ts pins defaultOptions.meta with and without a token. Nothing new to raise.

This is the unblocker for two red consumers. Confirmed against the current failing runs rather than assumed: alphafi-admin#34 typecheck fails with TS2353: 'grpcToken' does not exist in type 'AlphaFiSDKConfig' (plus two arity errors), and alphafi-fe#334 build fails with the same TS2353 in networkUtils.ts. Both resolve this SDK as a file:../ sibling at main, so both stay red until this merges.

One 🟢 carried, non-blocking and already noted on the PR: blockchain.ts still narrows options.network to testnet | mainnet, which the sibling alphalend-sdk-js#176 just fixed and documented on the identical line. The two SDKs now disagree on devnet — cheap follow-up, not a reason to hold this.

Approvals: 2/2 — merge is a maintainer call.

@11felix
11felix merged commit 70ecfc7 into main Aug 15, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants