Skip to content

make param optional - #169

Closed
OctoSauce wants to merge 1 commit into
mainfrom
bugfix/pyth-url
Closed

make param optional#169
OctoSauce wants to merge 1 commit into
mainfrom
bugfix/pyth-url

Conversation

@OctoSauce

Copy link
Copy Markdown
Contributor

No description provided.

@OctoSauce
OctoSauce requested review from 11felix and jangid August 1, 2026 06:53
@11felix
11felix self-requested a review August 1, 2026 11:25
@jangid

jangid commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

operate.md review — alphalend-sdk-js#169

First review on this PR (no prior comments). Re-derived against HEAD 2026-08-01. MERGEABLE / BLOCKED.

The change itself is clean

pythFullnodeUrl?: string on AlphalendClientOptions, consumed as options?.pythFullnodeUrl?.trim() || (<per-network default>). The .trim() || guard means whitespace-only and empty-string overrides correctly fall through to the default rather than producing an empty URL. The doc comment on src/core/types.ts:24-28 states the one thing a caller can get wrong — that it must be JSON-RPC, not gRPC/GraphQL. No findings.

🔴 Blocker for approval: CI is red — but not because of this PR

The test job fails in __tests__/oracle-price-validation.test.ts, which this diff does not touch:

● Oracle Price Validation › Root cause analysis: Oracle price entries validation
  Method not found. JSON-RPC on public fullnodes has been deprecated.
  Please migrate to gRPC or GraphQL endpoints.
  > 44 | const dynamicFields = await suiClient.getDynamicFields({

That is a pre-existing main breakage from Sui withdrawing JSON-RPC from public fullnodes, and alphalend-sdk-js#170 already fixes it by rewriting that test onto SuiGraphQLClient. #170 is now at 2/2 and merge-ready.

Action: merge #170, then rebase this branch — CI should go green with no change here. I'm withholding approval only because red CI is a hard gate, not because of anything in the diff.

🟡 Related, worth folding in while you're here

The per-network defaults this PR falls back to are themselves affected by the same shutdown. I checked all three directly:

default result
https://fullnode.testnet.sui.io/ (testnet) dead-32601 Method not found. JSON-RPC on public fullnodes has been deprecated.
https://fullnode.devnet.sui.io/ (devnet) dead — same error
https://alphalen-suimain-ef6f.mainnet.sui.rpcpool.com/ (mainnet) private Triton endpoint; returned 403 Access forbidden to an unauthenticated request from here, so I can't confirm either way — it is plausibly IP-allowlisted and fine in CI/prod

So on testnet and devnet, SuiPythClient cannot work at all unless the caller passes the new pythFullnodeUrl — this PR is the workaround, but the defaults it falls back to are broken for two of three networks. Worth either pointing those two at an endpoint that still serves JSON-RPC, or making the override required for non-mainnet so the failure is a clear construction-time error instead of a confusing -32601 at first use.

No approval this run — red CI. Everything above other than the CI gate is advisory.

@OctoSauce OctoSauce closed this Aug 2, 2026
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