Switch to object based RPC calls #22
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces several changes across the JavaScript client and its test suite. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Decorator
participant RPC
Client->>Decorator: Call getAsset(params)
Decorator->>Decorator: cleanInput(params)
Decorator->>RPC: getAsset({ id: ..., displayOptions: ... })
RPC-->>Decorator: Asset data or error
Decorator-->>Client: Asset data or throw DasApiError
sequenceDiagram
participant Client
participant Decorator
participant RPC
Client->>Decorator: Call getAssetsByOwner({ owner, ... })
Decorator->>Decorator: cleanInput({ ownerAddress: owner, ... })
Decorator->>RPC: getAssetsByOwner({ ownerAddress: ..., ... })
RPC-->>Decorator: Asset list or error
Decorator-->>Client: Asset list or throw DasApiError
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsclients/js/test/getAssetSignatures.test.ts (2)🧬 Code Graph Analysis (1)clients/js/test/getAssetSignatures.test.ts (1)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
clients/js/test/getAssetsByGroup.test.ts (1)
6-147: 💡 Verification agent🧩 Analysis chain
Verify test compatibility with getAssetSignatures
According to the PR objectives, the
getAssetSignaturesfunction currently only works with Aura as it relies ongetAssetSignaturesV2. Consider adding a test that verifies this compatibility or adding a warning comment to indicate this limitation.
🏁 Script executed:
#!/bin/bash # Check if there are tests for getAssetSignatures that specifically mention Aura compatibility grep -r "getAssetSignatures" --include="*.ts" clients/js/test/Length of output: 297
#!/bin/bash # Display the contents of the getAssetSignatures test file to check for Aura-specific tests or warnings sed -n '1,200p' clients/js/test/getAssetSignatures.test.ts
🏁 Script executed:
#!/bin/bash # Check for Aura references in the getAssetSignatures test grep -R "Aura" -n clients/js/test/getAssetSignatures.test.ts || echo "No references to Aura found" # Check for getAssetSignaturesV2 mentions in the test grep -R "getAssetSignaturesV2" -n clients/js/test/getAssetSignatures.test.ts || echo "No references to getAssetSignaturesV2 found"Length of output: 299
Add Aura-specific coverage for getAssetSignatures
The existinggetAssetSignatures.test.tssuite exercises the method but doesn’t document or verify its Aura‑only limitation (it relies ongetAssetSignaturesV2). Please add either:
- A test case against an Aura endpoint that asserts the function succeeds (and/or fails on non‑Aura chains), or
- A clear comment in the test or docs warning that
getAssetSignaturescurrently only works with Aura.Locations to update:
- clients/js/test/getAssetSignatures.test.ts
Example snippet to insert at the end of the file:
test('getAssetSignatures works on Aura and throws on non‑Aura', async (t) => { const auraUmi = createUmi(AURA_ENDPOINT_URL); const nonAuraUmi = createUmi(NON_AURA_ENDPOINT_URL); const params = { /* valid params */ }; // Aura should succeed await t.notThrowsAsync(() => auraUmi.rpc.getAssetSignatures(params)); // Non‑Aura should throw or return a meaningful error await t.throwsAsync(() => nonAuraUmi.rpc.getAssetSignatures(params)); });clients/js/test/getAssetSignatures.test.ts (1)
35-43: 💡 Verification agent🧩 Analysis chain
Inconsistency in test assertion.
The test expects to find 2 signatures (lines 35 and 37), but only verifies one signature (lines 38-43). The second signature's assertion seems to have been removed. This may cause the test to pass even if the second signature doesn't match expectations.
Run the following script to verify signature data:
🏁 Script executed:
#!/bin/bash # Check the actual response of getAssetSignatures to verify if it returns 2 signatures # Output the test file for reference echo "Checking test file:" cat clients/js/test/getAssetSignatures.test.ts # Check if there's an implementation file that might provide more context echo "Looking for related implementation files:" find clients/js/src -type f -name "*.ts" | xargs grep -l "getAssetSignatures"Length of output: 2112
Fix missing assertion for the second signature
In
clients/js/test/getAssetSignatures.test.ts, the second test case (“it can fetch the signatures of a compressed asset by leaf index…”) asserts that two signatures are returned:t.is(signatures.total, 2); t.is(signatures.limit, 1000); t.is(signatures.items.length, 2);but only verifies the first signature (
items[0]). You should add an assertion for the second signature to ensure its contents are also correct. For example:t.like(signatures.items[0], <DasApiTransactionSignature>{ signature: '2LzGEe1JZcf5SqtGhajrusvfRWSeL3Fm4sduQC1gbfPgMyQySf2tqS1yU5yTiHEutdUZkswHXHg6zzyjK6Vg92GA', instruction: 'MintToCollectionV1', slot: 356098299, }); + t.like(signatures.items[1], <DasApiTransactionSignature>{ + signature: '<expected_signature_2>', + instruction: '<expected_instruction_2>', + slot: <expected_slot_2>, + });Additionally, in the first test case (“it can fetch the signatures of a compressed asset by ID…”), the comment above the assertions still reads “Then we expect to find 2 signatures,” yet the code asserts
t.is(signatures.total, 1). Consider updating that comment to “1 signature” or adjusting the assertion if two were intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
clients/js/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
clients/js/aura.json(1 hunks)clients/js/package.json(1 hunks)clients/js/src/decorator.ts(6 hunks)clients/js/src/types.ts(1 hunks)clients/js/test/getAsset.test.ts(6 hunks)clients/js/test/getAssetProof.test.ts(1 hunks)clients/js/test/getAssetProofs.test.ts(1 hunks)clients/js/test/getAssetSignatures.test.ts(2 hunks)clients/js/test/getAssets.test.ts(7 hunks)clients/js/test/getAssetsByAuthority.test.ts(7 hunks)clients/js/test/getAssetsByCreator.test.ts(7 hunks)clients/js/test/getAssetsByGroup.test.ts(4 hunks)clients/js/test/getAssetsByOwner.test.ts(6 hunks)clients/js/test/searchAssets.test.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
clients/js/test/getAssetProof.test.ts (1)
clients/js/src/types.ts (1)
GetAssetProofRpcResponse(537-543)
clients/js/test/getAssetProofs.test.ts (1)
clients/js/src/types.ts (1)
GetAssetProofsRpcResponse(545-548)
clients/js/test/getAssetsByCreator.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi(17-18)
clients/js/test/getAssets.test.ts (2)
clients/js/src/types.ts (2)
DasApiAssetCompression(458-467)DasApiAssetContent(440-451)clients/js/test/_setup.ts (1)
createUmi(17-18)
clients/js/test/getAssetSignatures.test.ts (2)
clients/js/src/types.ts (1)
DasApiTransactionSignature(602-606)clients/js/test/_setup.ts (1)
createUmi(17-18)
clients/js/test/getAsset.test.ts (2)
clients/js/src/types.ts (2)
DasApiAssetCompression(458-467)DasApiAsset(241-306)clients/js/test/_setup.ts (1)
createUmi(17-18)
clients/js/test/searchAssets.test.ts (1)
clients/js/src/types.ts (1)
DasApiAsset(241-306)
🪛 Biome (1.9.4)
clients/js/test/getAssets.test.ts
[error] 38-38: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 48-48: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 49-49: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 53-53: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 54-54: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 57-57: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 58-58: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 59-59: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🔇 Additional comments (42)
clients/js/src/types.ts (1)
421-424: Good export of the DasApiParamAssetSortBy typeThis change exports the DasApiParamAssetSortBy type, making it available for external use. This aligns well with the PR objective of switching to object-based RPC calls by enabling consistent typing for sort parameters across the codebase.
clients/js/aura.json (1)
1-2: Good addition of sample token metadata for testingThis new JSON file provides comprehensive test data with detailed metadata for six tokens (five NFTs and one fungible token). It contains all the necessary fields like interface types, asset IDs, content metadata, authorities, compression details, royalty information, and ownership details that will support the new object-based RPC call implementation.
clients/js/package.json (1)
38-38: Appropriate dependency version updateUpdating the @metaplex-foundation/umi dependency to "^1.2.0" is appropriate for supporting the transition to object-based RPC calls.
clients/js/test/getAssetsByGroup.test.ts (4)
12-13: Updated test data to match new sample token metadataThe public key for the group value has been updated to align with the new sample data in aura.json.
39-40: Updated group value for consistencyThe public key has been updated to maintain consistency with the new test data structure.
69-70: Consistent test data updateThis is the same public key as used in the previous test case, which is appropriate since both tests are checking different aspects (verified vs. unverified collections) of the same collection.
100-100: Updated collection metadata test referenceThe public key for testing collection metadata display has been updated to match the new sample data.
clients/js/test/getAssetProof.test.ts (2)
10-10: Test data update looks correct.The asset ID has been updated to use a new value, which matches the broader test data refreshes mentioned in the summary.
17-28: Expected test data properly updated to match the new asset.The expected response data has been updated to match the new asset ID, including root, proof array elements, node index, leaf, and tree ID values.
clients/js/test/getAssetsByOwner.test.ts (5)
10-10: Owner public key updated correctly.The owner public key has been updated to a new value, which is part of the broader test data refresh across the codebase.
31-31: Owner public key updated correctly.The owner public key has been updated for the test case with
showUnverifiedCollectionsset to true.
65-65: Owner public key updated correctly.The owner public key has been updated for the test case with
showCollectionMetadataset to true.
89-89: Collection value string updated correctly.The target collection value string has been updated to match the new test data.
120-120: Owner public keys updated consistently.The owner public keys have been updated for both test cases with
showFungibleset to true and false. Both tests now use the same owner, which maintains consistency.Also applies to: 155-155
clients/js/test/getAssetProofs.test.ts (3)
11-13: Asset IDs updated correctly.The asset IDs have been updated to new values, which is part of the broader test data refresh across the codebase.
20-31: Expected test data properly updated for the first asset.The expected response data has been updated to match the new asset ID, including root, proof array elements, node index, leaf, and tree ID values.
34-45: Expected test data properly updated for the second asset.The expected response data has been updated to match the new asset ID, including root, proof array elements, node index, leaf, and tree ID values.
clients/js/test/getAssetSignatures.test.ts (3)
10-10: Asset ID updated correctly.The asset ID has been updated to a new value, which is part of the broader test data refresh across the codebase.
16-17: Expected signature count updated correctly.The total number of signatures has been updated from 2 to 1, which matches the new test data.
29-30: Tree public key updated correctly.The tree public key has been updated to a new value, which is part of the broader test data refresh.
clients/js/test/getAssetsByAuthority.test.ts (1)
77-89: Improved verification logic for collection validation.The updated approach correctly handles the case where an asset might not have any grouping information or where a collection grouping might not have a
verifiedproperty. This makes the test more robust when dealing with diverse asset structures.clients/js/test/getAsset.test.ts (1)
72-92:✅ Verification successful
Test behavior changed from expecting an error to expecting success with empty grouping.
This test previously appears to have expected an error when
showUnverifiedCollectionswas false, but now it expects a successful response with an empty grouping array. This seems to reflect a change in how the API handles unverified collections - now returning the asset but without the unverified collection information rather than failing entirely.
🏁 Script executed:
#!/bin/bash # Verify how the implementation handles showUnverifiedCollections rg -A 10 "showUnverifiedCollections" clients/js/src/Length of output: 563
🏁 Script executed:
#!/bin/bash # Locate getAsset implementation rg -l "export async function getAsset" clients/js/src # Inspect implementation for handling of showUnverifiedCollections and grouping GFILE=$(rg -l "export async function getAsset" clients/js/src) echo "---- Start of $GFILE ----" sed -n '1,200p' "$GFILE" echo "---- Searching for 'unverified' across implementation ----" rg -n "unverified" clients/js/srcLength of output: 471
Approve updated test behavior for showUnverifiedCollections
The test now correctly reflects that hiding unverified collections no longer triggers a client‐side error but returns the asset with an empty
groupingarray. We’ve confirmed inclients/js/src/types.tsthatshowUnverifiedCollectionsis an optional display option passed through to the API, and there’s no local error being thrown.No changes required.
clients/js/test/getAssetsByCreator.test.ts (2)
42-49: Improved verification logic for unverified creators.The updated test now correctly checks for at least one asset with an unverified creator, rather than assuming all creators should be unverified. This more accurately tests the expected behavior.
129-133: Improved verification logic for collection groups.The updated assertion is more precise - it checks that no collection has
verified: falserather than asserting all collections must haveverified: true. This handles the case where some collection groups might not have averifiedproperty at all.clients/js/test/searchAssets.test.ts (1)
22-22: Relaxed assertion for number of found assets.The change from
> 1to>= 1makes the test more robust, allowing it to pass even when only a single matching asset is found.clients/js/test/getAssets.test.ts (7)
10-14: Asset IDs have been updated to match new test data.The test data has been updated with new public keys for the compressed and regular assets. This change aligns with the PR's objective to improve compatibility with different RPC implementations.
32-37: Updated test asset content with generic example data.The compressed asset's JSON URI and metadata name were changed to use more generic example values, which makes the tests more maintainable.
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
38-47: Updated compression-related hash values and tree identifiers.The compression-related hash values and tree identifiers have been updated to match the new test data structure. This ensures consistency with the sample data in the referenced
aura.jsonfile.🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
56-63: Updated regular asset interface type and metadata.The regular asset's interface type has been changed from "V1_NFT" to "ProgrammableNFT" and its metadata name updated to "Mad Lads #4221", reflecting the change to using more specific named examples.
🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 58-58: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 59-59: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
67-71: Updated collection reference for the regular asset.The collection group_value has been updated to maintain consistency with the new asset data being tested.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 68-68: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
99-104: Added verification flag for unverified collections.The test now explicitly checks for a
verified: falseflag in the asset grouping when using theshowUnverifiedCollectionsoption. This improves test coverage by verifying this specific field.
138-148: Updated collection metadata with specific collection details.The collection metadata has been updated with real-world example data (Mad Lads collection), which makes the tests more representative of actual use cases.
clients/js/src/decorator.ts (10)
98-107: Good addition of a utility function to clean input objects.The
cleanInputfunction elegantly removes null values and empty objects from the input parameters. This improves robustness when passing arguments to RPC calls and follows the principle of sending only meaningful data.
134-140: Switched from positional to object-based RPC arguments.This change from positional arguments to named objects aligns with the PR objectives and improves compatibility with different RPC implementations. The code is now more maintainable and type-safe.
150-156: Improved clarity in getAssets RPC call with named parameters.The change to using named parameters (
idsandoptions) makes the code more readable and less prone to errors when the order of arguments changes.
161-164: Consistent parameter naming in getAssetProof.The change to use
idinstead of passing the asset ID directly maintains consistency with the object-based approach used throughout the file.
179-192: Enhanced type safety with descriptive parameter names.Renaming from
authoritytoauthorityAddressmakes the API more self-documenting and consistent with other address parameters. The use ofcleanInputensures only relevant data is sent.
202-216: Improved getAssetsByCreator with named parameters.Similar to other methods, this change improves the API by using descriptive parameter names and cleaning the input before making the RPC call.
224-238: Consistent pattern applied to getAssetsByGroup.The same pattern of named parameters and input cleaning is applied consistently across all methods.
248-261: Named parameter pattern for getAssetsByOwner.The change from
ownertoownerAddressimproves the API's clarity and consistency.
269-303: Comprehensive parameter mapping for searchAssets.The searchAssets method has many parameters, and the object-based approach makes it much clearer which parameters are being passed, especially with the renaming to more descriptive names.
17-17: Type export for DisplayOptions is now used consistently.Adding the DisplayOptions type to the imports indicates it's being used more extensively throughout the codebase, which improves type safety and makes the API more consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (2)
clients/js/src/decorator.ts(5 hunks)clients/js/test/getAssetSignatures.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
clients/js/test/getAssetSignatures.test.ts (1)
clients/js/test/_setup.ts (2)
DAS_API_ENDPOINTS(5-5)createUmi(17-18)
clients/js/src/decorator.ts (2)
clients/js/src/types.ts (8)
DasApiAsset(241-306)DisplayOptions(10-27)GetAssetsRpcInput(222-232)GetAssetsByAuthorityRpcInput(29-39)GetAssetsByGroupRpcInput(58-73)SearchAssetsRpcInput(87-202)GetAssetSignaturesRpcInput(550-600)GetAssetSignaturesRpcResponse(608-633)clients/js/src/errors.ts (1)
DasApiError(1-3)
🔇 Additional comments (12)
clients/js/test/getAssetSignatures.test.ts (3)
10-10: Update of test asset IDs looks goodThe asset and tree IDs have been updated to match the new test data used across the test suite. This ensures consistency with other test files that were also updated as part of this refactoring.
Also applies to: 27-28
15-21: Test assertions correctly updated for new response formatThe test assertions have been properly updated to reflect the change in expected response structure. The test now correctly checks for a single signature (total of 1) and verifies the content using
deepEqualto match both the signature string and instruction name.
32-39: Consistent test approach for leaf index testGood consistency between both test cases. The leaf index test now also expects a single signature and uses the same assertion pattern as the asset ID test, which improves test readability and maintenance.
clients/js/src/decorator.ts (9)
98-108: Utility function for cleaning inputs is well-implementedThe
cleanInpututility function is a good addition that removes null, undefined, and empty object properties from input objects. This helps create cleaner RPC request payloads. The implementation is concise and reusable across all methods.Note that this implementation incorporates the feedback from the previous review by danenbm to handle undefined values directly in the function rather than adding them and then cleaning them out.
135-141: Improved type safety for getAsset RPC callThe refactoring from positional arguments to object-based parameters improves type safety and readability. The explicit type annotations make it clear what the RPC call expects and returns.
151-157: Enhanced getAssets with consistent parameter structureSimilarly to
getAsset, this method now uses a consistent object-based structure for the RPC call, improving maintainability and type checking.
162-165: Proof methods updated to use consistent object parameter patternBoth
getAssetProofandgetAssetProofsmethods have been updated to the new pattern, maintaining consistency across the API interface.Also applies to: 170-173
180-193: Improved parameter handling for getAssetsByAuthorityThe method now creates a cleaned input object with renamed keys that are more descriptive (
authority→authorityAddress). This enhances compatibility with different RPC implementations as mentioned in the PR description.
203-217: Consistent parameter handling across all asset-fetching methodsGood consistency in how
getAssetsByCreator,getAssetsByGroup, andgetAssetsByOwnerare refactored. Each method follows the same pattern of:
- Validating pagination
- Creating a cleaned input object with renamed keys
- Making the RPC call with the cleaned input
- Handling the response with proper error checking
This consistency makes the codebase more maintainable.
Also applies to: 225-239, 249-262
270-304: Comprehensive parameter mapping for searchAssetsThe
searchAssetsmethod handles a large number of parameters, and the refactoring correctly maps all of them to their respective keys in the cleaned input object. This is particularly important for this method since it has the most complex parameter set.
1-338: Overall implementation of object-based RPC calls is well structuredThe refactoring from position/array-based RPC calls to object-based calls is comprehensive and consistent across all methods. This change improves:
- Type safety - each RPC call now has explicit parameter and return types
- Readability - parameter names are clearer and more consistent
- Compatibility - as mentioned in the PR objectives, this supports RPC implementations that differ from the reference implementation
The utility function
cleanInputis a nice addition that reduces code duplication and ensures consistent handling of input parameters.The PR achieves its stated objective of transitioning from position/array-based RPC calls to object-based RPC calls while maintaining backward compatibility with the existing API interface.
312-326:❓ Verification inconclusive
Enhanced getAssetSignatures with flexible parameter handling
The method now properly handles both asset ID and tree/leaf index inputs more cleanly, using the
cleanInputfunction to remove empty values. Parameter names are also converted to a more consistent format (leaf_index→leafIndex,sort_direction→sortDirection).
🏁 Script executed:
#!/bin/bash # Search for references to getAssetSignaturesV2 to verify RPC compatibility rg -A 5 'getAssetSignatures'Length of output: 3822
🏁 Script executed:
#!/bin/bash # Show parameter definitions for getAssetSignatures in the API spec rg -C 10 '"getAssetSignatures"' specification/metaplex-das-api.jsonLength of output: 620
#!/bin/bash # Display full parameter lists for getAssetSignatures (and any V2 variant) in the API spec rg -A 50 '"name": "getAssetSignatures"' specification/metaplex-das-api.json rg -A 10 '"name": "getAssetSignaturesV2"' specification/metaplex-das-api.json
Verify new getAssetSignatures parameters with API spec
The updated getAssetSignatures call now:
- Uses cleanInput to strip out null or undefined values
- Supports either
{ assetId }or{ tree, leafIndex }- Renames
leaf_index→leafIndexandsort_direction→sortDirectionfor consistency- Preserves pagination and cursor options (
limit,page,before,after,cursor)Please confirm that your RPC schema in
specification/metaplex-das-api.jsonforgetAssetSignaturesactually declares matching parameters for:
treeleafIndexcursorsortDirection—or update the spec if any are missing—to ensure compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (1)
clients/js/test/getAssetSignatures.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarkSackerberg
PR: metaplex-foundation/digital-asset-standard-api#22
File: clients/js/test/getAssetProofs.test.ts:16-16
Timestamp: 2025-04-22T09:44:46.015Z
Learning: For the PR transitioning from position/array-based RPC calls to object-based RPC calls in the digital-asset-standard-api, methods with only one parameter (like `getAssetProofs`) can remain using the direct parameter style rather than being converted to object-based.
🧬 Code Graph Analysis (1)
clients/js/test/getAssetSignatures.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi(17-18)
🔇 Additional comments (4)
clients/js/test/getAssetSignatures.test.ts (4)
9-9: Asset ID updated for new test data.This change updates the asset ID used in the test to reflect the refreshed test data mentioned in the PR summary. The new asset ID appears to be consistent with the broader test suite updates.
14-15: Expected signature count reduced from 2 to 1.The test now expects 1 signature instead of 2. This change aligns with the PR's mention that
getAssetSignaturesrelies ongetAssetSignaturesV2and currently only works with Aura.It might be helpful to add a comment explaining this reduction in expected signatures, especially if it's related to the API behavior changes or compatibility differences between RPC implementations.
26-26: Tree public key updated for new test data.This change updates the tree public key used in the test to reflect the refreshed test data mentioned in the PR summary.
31-32: Expected signature count reduced from 2 to 1.Similar to the first test, this test now expects 1 signature instead of 2, maintaining consistency between the two test cases.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (1)
clients/js/test/getAssetSignatures.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarkSackerberg
PR: metaplex-foundation/digital-asset-standard-api#22
File: clients/js/test/getAssetProofs.test.ts:16-16
Timestamp: 2025-04-22T09:44:46.015Z
Learning: For the PR transitioning from position/array-based RPC calls to object-based RPC calls in the digital-asset-standard-api, methods with only one parameter (like `getAssetProofs`) can remain using the direct parameter style rather than being converted to object-based.
🧬 Code Graph Analysis (1)
clients/js/test/getAssetSignatures.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi(17-18)
🔇 Additional comments (5)
clients/js/test/getAssetSignatures.test.ts (5)
9-9: Test data updated appropriatelyThe updated asset ID and tree public key align with the PR's test data refresh to maintain consistency across the test suite.
Also applies to: 26-26
12-12: Correctly implemented object-based RPC callsThe RPC calls now use object-based arguments (
{ assetId }and{ tree, leaf_index }) instead of positional array arguments, which successfully implements the PR's main objective of switching to object-based RPC calls.Also applies to: 29-29
14-15: Updated expected signature countThe test has been appropriately updated to expect 1 signature instead of 2, which aligns with the updated test data.
Also applies to: 31-32
17-20: Simplified signature assertion with proper type annotationThe signature assertions have been simplified to check for a two-element array containing a signature string and instruction name, and include the type assertion
as [string, string]as suggested in previous review comments.Also applies to: 35-38
34-34: Added explicit length checkGood addition to explicitly verify the length of the items array, which provides an additional verification point and improves test robustness.
Instead of using position / array based RPC calls the SDK uses objects now.
This makes it more compatible to RPCs that are not competely the same as the reference implementation.
getAssetSignatures unfortunately is only working with Aura right now since it uses getAssetSignaturesV2