Conversation
11felix
commented
Apr 26, 2026
- Migrated SDK read paths from JSON-RPC to GraphQL while preserving existing public behavior and return contracts.
- Refactored blockchain/query parsing to handle GraphQL-native object/dynamic field/event shapes with stricter typing.
- Updated client architecture to keep transaction building reliable while removing unnecessary external SuiClient coupling.
- Added migration snapshot + semantic diff tooling to validate parity across all exposed reads and transaction builders.
- Documented migration + API updates and aligned quality gates (lint/type/test/pre-commit) for merge-ready stability.
jangid
left a comment
There was a problem hiding this comment.
Review: JSON-RPC to GraphQL Migration
Overall: Well-executed migration. Architecture is sound — all reads move to SuiGraphQLClient, public API simplifies, and the snapshot-diff tooling is a thoughtful approach to verifying parity. CI is green.
Issues to Address
1. Breaking change: getCoinObject for SUI with amount (behavior change)
In blockchain.ts, the new getCoinObject for SUI when amount is provided:
if (this.isCoinTypeSui(coinType)) {
if (amount) {
return tx.splitCoins(tx.gas, [amount]);
}
return tx.gas;
}The old code in client.ts always returned tx.gas for SUI regardless of amount. The new code splits when amount is provided. This changes transaction shape for SUI supply/borrow/repay flows — the snapshot diff tool might mask this if it's categorized as noise. Verify this is intentional and tested end-to-end.
2. multiGetObjects query — ObjectKey type mismatch
In blockchain.ts:
variables: { objectIds: batch.map((id) => ({ address: id })) },The Sui GraphQL schema's multiGetObjects takes [ObjectKey\!]\! which requires { address, version }, not just { address }. If this works at all, it's only because the server is lenient. Confirm this actually functions — or use individual getObject calls / the objectConnection query instead.
3. interestRates type assertion is unsafe
In parser.ts:
interestRates: config.interest_rates as unknown as number[],interest_rates might also come as base64 from GraphQL (same as interest_rate_kinks). The type in queryTypes.ts says string[] | number[] but doesn't handle the base64 case. Should this also use decodeU8Vec?
4. Receipt type is a breaking change for downstream consumers
The old Receipt had receipt.content.type and receipt.content.fields.*. The new shape is { objectId, fields: ReceiptGql }. Any consumer accessing receipt.content will break. The README migration table doesn't mention this. Consider documenting it or keeping a compatibility shim.
5. Missing getEstimatedGasBudget signature change in exports
The old signature was getEstimatedGasBudget(suiClient, tx, address), the new is getEstimatedGasBudget(tx, address). Listed in README — good. But verify this is reflected in the published type declarations (index.ts exports).
Minor / Nits
testGetAllMarkets.ts:const clientDuration = 0;is dead code left from removing SuiClient creation timing. Remove it and references in benchmark output.pythTest.ts: Changed from// run()torun().catch(...)— this makes the script auto-execute on import. Intentional?oracle-price-validation.test.ts: Still importsSuiClientand constructs one manually for raw dynamic field probing — the test still has a JSON-RPC dependency. Worth noting.tsconfig.json: New roottsconfig.jsonextendstsconfig.esm.json— ensure no conflict withtsconfig.cjs.jsonbuild path.
Questions
- Was the snapshot diff tool actually run, and what was the result? (0 signal diffs?)
- Is this intended as a semver major bump (2.0.0)? README references "2.x" but no
package.jsonversion change in the diff. - The
priceIdentifier.typefield removal — are there any known downstream consumers inalphafi-api,alphafi-crons, oralphalend-api?
Questions -
|
jangid
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. Responses are satisfactory — intentional SUI split behavior, multiGetObjects confirmed working, interest_rates as number[] verified, README updated for Receipt and getEstimatedGasBudget changes. 0 signal diffs on snapshot tool is reassuring.
Minor nits (dead clientDuration, pythTest auto-execute, test JSON-RPC dep) can be cleaned up in a follow-up. Bump to 2.0.0 on merge.