gRPC auth support: grpcToken through coinHelpers and Blockchain (explicit transport) - #176
Conversation
|
operate.md review — head ✅ Both findings resolvedRe-derived against the current diff, not taken on the reply:
The live A/B against BlockPI ( 🟢 Non-blocking
One correction on the merge storyYour note says the base was retargeted and GitHub will retarget to CI green (test / lint / build). Not a contract PR. Approving — that takes this to 1/2, so it needs one more approval before merge. Prior findings on this PR are resolved or deliberately deferred as noted; nothing remains open. |
|
@Zorag44 Both findings confirmed and fixed in 0af5ec8. 🔴 meta dropped: reproduced per your method (constructor source + live A/B: client-level 🟡 devnet narrowing: fixed — 🟢 On the non-blocking notes: the token-threading shape question is resolved by this PR + #174 sharing one |
Zorag44
left a comment
There was a problem hiding this comment.
operate.md: clean — both findings resolved (explicit transport at src/models/blockchain.ts:76-88 and src/core/coinHelpers.ts:304-321; devnet network passthrough at :83), test confirmed running in CI. Non-blocking notes in the summary comment. Approvals: 1/2 — needs 1 more before merge.
jangid
left a comment
There was a problem hiding this comment.
operate.md: clean — CI green (test / lint / build), head 0af5ec8. Not a contract PR. Zero review threads open; both findings from the earlier pass are closed on the record and I found nothing new.
Re-derived this run: meta rides an explicitly constructed GrpcWebFetchTransport (with baseUrl moved onto the transport) at both construction sites, @protobuf-ts/grpcweb-transport is a declared direct dependency at ^2.11.1 matching @mysten/sui's own range with a single hoisted resolution in the lockfile, network passes through unchanged so devnet is no longer narrowed, and __tests__/grpc-transport-auth.test.ts pins defaultOptions.meta at both the Blockchain and AlphalendClient layers plus the absent-token case.
This is a merge-order unblocker. alphafi-fe#334 currently fails its build with TS2353: 'grpcUrl' does not exist in type 'AlphalendClientOptions' — verified in the failing run for bf89ac66 this run — because the FE resolves this SDK as a file:../ sibling at main. That build goes green only once this merges.
Approvals: 2/2 — merge is a maintainer call.
Consolidates #174 into this PR (same body of work — #174 is closed in its favor; its commits are included here).
What this adds:
getCoinObjectCounts/buildMergeCoinsTransactiontake an optionalgrpcToken(alphafi-admin's merge flow needs it for BlockPI).AlphalendClientOptionsgainsgrpcUrl/grpcToken, threaded intoBlockchain, which now exposes a configuredsuiGrpcClient(mirrors alphafi-sdk-js#99). SDK reads stay on GraphQL — this is plumbing for consumers and future gRPC reads.The critical part — explicit transport (Zorag44's blocking finding):
SuiGrpcClient's convenience constructor forwards onlybaseUrl/fetchInitto the transport it builds and silently dropsmeta, so a client-level token is never sent. Both construction sites here buildGrpcWebFetchTransportexplicitly (@protobuf-ts/grpcweb-transportadded as a direct dependency,^2.11.1matching@mysten/sui's own range). Also fixes the devnet-narrowing issue:networkpasses through unchanged.Verified: 61/61 tests including
__tests__/grpc-transport-auth.test.ts(pinsdefaultOptions.metaat both theBlockchainandAlphalendClientlayers), plus live authenticated reads against BlockPI through the builtAlphalendClient(client-levelmeta→Apikey not found; this shape → success). No behavior change when the options are omitted.