-
Notifications
You must be signed in to change notification settings - Fork 16
MET-685: Add new parameters #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary by CodeRabbit
WalkthroughThe updates modify the asset API by changing method signatures in the decorator and type definitions to accept structured input objects that include additional properties such as display options. The pagination validation is now encapsulated in a dedicated function. New types—including display options, asset input types, and token types—have been introduced, and several existing type definitions have been refactored. Across the test suite, asset IDs, expected interfaces, and assertion logic have been updated and expanded to cover scenarios like unverified collections and collection metadata. The test setup now also includes extra checks for valid endpoint configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as DasApiDecorator
participant V as validatePagination
participant R as RPC Service
U ->> D: getAsset({ assetId, displayOptions })
alt Pagination parameters present
D ->> V: validatePagination(params)
end
D ->> R: Request asset data with assetId & displayOptions
alt Asset found
R -->> D: Return asset data
D -->> U: Return asset data
else Asset missing
R -->> D: Return error/empty response
D -->> U: Throw DasApiError
end
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (5)
clients/js/test/searchAssets.test.ts (1)
1-146
:⚠️ Potential issueFix code style issues
There are code style issues reported by the pipeline. Please run Prettier on the file to ensure consistent formatting.
#!/bin/bash # Run Prettier on this file to fix the formatting issues npx prettier --write clients/js/test/searchAssets.test.ts🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/test/getAssetsByCreator.test.ts (1)
1-184
: 💡 Verification agent🧩 Analysis chain
Run Prettier to fix code style issues.
The pipeline has flagged code style issues. Please run Prettier on this file to ensure consistent formatting.
🏁 Script executed:
#!/bin/bash # Check if prettier is installed and run it on the file if command -v npx &> /dev/null; then echo "Running Prettier check on getAssetsByCreator.test.ts" npx prettier --check "clients/js/test/getAssetsByCreator.test.ts" else echo "npx not found. Please ensure npm is installed." fiLength of output: 442
Action Required: Format Code with Prettier
The Prettier check flagged formatting issues in
clients/js/test/getAssetsByCreator.test.ts
. Please run the following command to automatically fix the formatting:npx prettier --write clients/js/test/getAssetsByCreator.test.ts
This will update the file to adhere to our project's style guidelines.
🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: 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)
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/test/getAssets.test.ts (2)
28-72
: 🧹 Nitpick (assertive)Replace non-null assertions with optional chaining.
Multiple uses of non-null assertions (
!
) were flagged by static analysis. These should be replaced with optional chaining (?.
) for better type safety.For example:
-t.deepEqual(compressedAsset!.interface, 'V1_NFT'); +t.deepEqual(compressedAsset?.interface, 'V1_NFT');This should be applied to all instances of non-null assertions in the file (lines 28-29, 30, 37, 47-48, 52-53, 56-58, 63, 66-67, 71-72).
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: 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] 29-29: 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] 30-30: 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] 37-37: 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] 47-47: 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] 52-52: 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] 56-56: 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] 63-63: 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] 66-66: 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] 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] 71-71: 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] 72-72: 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)
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
1-173
: 💡 Verification agent🧩 Analysis chain
Run Prettier to fix code style issues.
The pipeline has flagged code style issues. Please run Prettier on this file to ensure consistent formatting.
🏁 Script executed:
#!/bin/bash # Check if prettier is installed and run it on the file if command -v npx &> /dev/null; then echo "Running Prettier check on getAssets.test.ts" npx prettier --check "clients/js/test/getAssets.test.ts" else echo "npx not found. Please ensure npm is installed." fiLength of output: 318
Action Required: Fix Prettier Formatting Issues
The file
clients/js/test/getAssets.test.ts
currently has code style issues as flagged by Prettier. Please run the following command to automatically fix the formatting:npx prettier --write clients/js/test/getAssets.test.tsOnce formatted, ensure that the file passes the Prettier check before committing.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: 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] 29-29: 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] 30-30: 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] 37-37: 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] 47-47: 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] 52-52: 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] 56-56: 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] 63-63: 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] 66-66: 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] 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] 71-71: 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] 72-72: 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)
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/src/decorator.ts (1)
1-306
:⚠️ Potential issueRun Prettier to fix code style issues.
The pipeline is reporting code style issues. Please run Prettier to ensure consistent formatting.
#!/bin/bash # Run Prettier to fix formatting issues npx prettier --write clients/js/src/decorator.ts🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
clients/js/src/decorator.ts
(2 hunks)clients/js/src/types.ts
(9 hunks)clients/js/test/_setup.ts
(1 hunks)clients/js/test/getAsset.test.ts
(2 hunks)clients/js/test/getAssets.test.ts
(2 hunks)clients/js/test/getAssetsByAuthority.test.ts
(2 hunks)clients/js/test/getAssetsByCreator.test.ts
(1 hunks)clients/js/test/getAssetsByGroup.test.ts
(1 hunks)clients/js/test/getAssetsByOwner.test.ts
(1 hunks)clients/js/test/searchAssets.test.ts
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
clients/js/test/getAssetsByGroup.test.ts (2)
clients/js/test/_setup.ts (1)
createUmi
(17-18)clients/js/src/types.ts (1)
DasApiAsset
(241-306)
clients/js/test/getAssetsByAuthority.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi
(17-18)
clients/js/test/searchAssets.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi
(17-18)
clients/js/test/getAssets.test.ts (1)
clients/js/src/types.ts (2)
DasApiAssetContent
(440-451)DasApiAssetCompression
(458-467)
clients/js/src/decorator.ts (2)
clients/js/src/types.ts (11)
GetAssetRpcInput
(207-217)DasApiAsset
(241-306)GetAssetsRpcInput
(222-232)GetAssetsByAuthorityRpcInput
(29-39)DasApiAssetList
(355-377)GetAssetsByCreatorRpcInput
(41-56)GetAssetsByGroupRpcInput
(58-73)GetAssetsByOwnerRpcInput
(75-85)SearchAssetsRpcInput
(87-202)GetAssetSignaturesRpcInput
(550-600)GetAssetSignaturesRpcResponse
(608-633)clients/js/src/errors.ts (1)
DasApiError
(1-3)
clients/js/test/getAsset.test.ts (2)
clients/js/src/types.ts (1)
DasApiAsset
(241-306)clients/js/test/_setup.ts (1)
createUmi
(17-18)
clients/js/test/getAssetsByCreator.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi
(17-18)
clients/js/test/getAssetsByOwner.test.ts (2)
clients/js/test/_setup.ts (1)
createUmi
(17-18)clients/js/src/types.ts (1)
DasApiAsset
(241-306)
🪛 GitHub Actions: Main
clients/js/test/searchAssets.test.ts
[warning] Code style issues found. Forgot to run Prettier?
clients/js/test/getAssets.test.ts
[warning] Code style issues found. Forgot to run Prettier?
clients/js/src/decorator.ts
[warning] Code style issues found. Forgot to run Prettier?
clients/js/test/getAssetsByCreator.test.ts
[warning] Code style issues found. Forgot to run Prettier?
🪛 Biome (1.9.4)
clients/js/test/getAssets.test.ts
[error] 28-28: 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] 29-29: 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] 30-30: 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] 37-37: 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] 47-47: 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] 52-52: 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] 56-56: 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] 63-63: 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] 66-66: 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] 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] 71-71: 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] 72-72: 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)
clients/js/test/getAssetsByCreator.test.ts
[error] 166-166: 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)
clients/js/test/getAssetsByOwner.test.ts
[error] 96-96: 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 (53)
clients/js/test/_setup.ts (1)
10-13
: Good defensive programming practice!This change improves robustness by checking if the URL exists before adding it to the endpoints array, preventing potential runtime errors if environment variables are undefined.
clients/js/test/getAssetsByGroup.test.ts (3)
34-65
: LGTM: Good test for hiding unverified collections!This test properly validates that when
showUnverifiedCollections
is set tofalse
, only verified collections are included in the results.
67-100
: LGTM: Good test for showing unverified collections!This test properly validates that when
showUnverifiedCollections
is set totrue
, both verified and unverified collections are included in the results.
102-155
: LGTM: Thorough validation of collection metadata!This test comprehensively checks the
showCollectionMetadata
option by verifying the presence and correct types of essential metadata fields (name, description, image, symbol).clients/js/test/getAssetsByAuthority.test.ts (4)
12-15
: LGTM: Good refactoring to object parameter style!Nice update to use an object parameter, which makes the code more consistent with the other new tests and provides better extensibility for future parameters.
24-55
: LGTM: Good test for showing unverified collections!This test properly validates that when
showUnverifiedCollections
is set totrue
, assets with unverified collections are included in the results.
57-88
: LGTM: Good test for excluding unverified collections!This test properly validates that when
showUnverifiedCollections
is set tofalse
, only verified collections are included in the results.
90-137
: LGTM: Comprehensive test for collection metadata!This test thoroughly validates the
showCollectionMetadata
option by checking for the presence of specific metadata fields and their values.clients/js/test/searchAssets.test.ts (3)
72-72
: Good catch on fixing the comment!Corrected "assetw" to "assets" in the comment for better readability.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
93-122
: LGTM: Good test for displaying unverified collections!This test correctly validates the behavior when
showUnverifiedCollections
is true, ensuring the specified asset is included in the results.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
124-145
: LGTM: Good test for hiding unverified collections!This test correctly validates that when
showUnverifiedCollections
is false, assets that are not verified parts of collections are excluded from the results.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/test/getAsset.test.ts (4)
51-51
: Asset ID updated to use a different test NFT.The test now uses the asset ID
Hu9vvgNjVDxRo6F8iTEo6sRJikhqoM2zVswR86WAf4C
instead of the previous one, which is likely a change to maintain test consistency with the new display options functionality.
58-58
: Test assertions updated to match current asset properties.The expected interface has been updated from 'ProgrammableNFT' to 'V1_NFT', and explicit assertions for
mutable
andburnt
properties have been added, which improves test clarity and coverage.Also applies to: 69-72
73-90
: Added test for showUnverifiedCollections=false.This test properly verifies that the API throws an appropriate error when attempting to fetch an asset with an unverified collection when
showUnverifiedCollections
is set tofalse
. This aligns with the expected behavior for this new parameter.
92-117
: Added test for showUnverifiedCollections=true.This test appropriately verifies that assets with unverified collections are returned when
showUnverifiedCollections
is set totrue
. The test includes proper assertions for the unverified collection in the grouping data.clients/js/test/getAssetsByCreator.test.ts (5)
27-50
: Added test for fetching assets by non-verified creators.This test correctly verifies that assets can be fetched with
onlyVerified: false
and checks that the creator is present but not verified in the returned assets. This provides good coverage for the non-verified creators use case.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
52-66
: Added test for verified creators filtering.The test properly verifies that when
onlyVerified: true
is specified with a non-verified creator, no assets are returned, which validates the filtering behavior.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
68-101
: Test for showUnverifiedCollections=true option.This test correctly verifies that assets with unverified collections are included in the results when the
showUnverifiedCollections
flag is set to true. The test also ensures that at least one asset with an unverified collection exists in the returned results.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
103-131
: Test for showUnverifiedCollections=false option.Good implementation of the test that verifies all returned collections are verified when
showUnverifiedCollections
is set to false. This addresses the previous review comment that requested verification that all collections are verified.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
133-183
: Test for showCollectionMetadata option.This test correctly validates that collection metadata is included in the results when
showCollectionMetadata
is set to true. It properly checks for the presence and structure of the collection metadata.🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: 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)
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/test/getAssetsByOwner.test.ts (5)
28-60
: Added test for showUnverifiedCollections=true option.This test correctly verifies that assets with unverified collections are included in the results when
showUnverifiedCollections
is enabled. The test properly confirms that at least one asset with an unverified collection exists in the response.
62-113
: Added test for showCollectionMetadata option.This test appropriately validates that collection metadata is included in the results when
showCollectionMetadata
is set to true. It correctly asserts the presence and structure of the collection metadata.🧰 Tools
🪛 Biome (1.9.4)
[error] 96-96: 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)
107-110
: Acknowledged TODO comment.The TODO comment regarding missing description and external_url fields is acknowledged. According to the previous review discussion, this is blocked by MTG-1493 and will be addressed in a follow-up PR, which is a reasonable approach.
115-148
: Added test for showFungible=true option.The test correctly verifies that fungible token assets are included in the results when
showFungible
is set to true. It properly checks that at least one fungible token asset is present in the response.
150-180
: Added test for showFungible=false option.This test appropriately validates that fungible token assets are excluded from the results when
showFungible
is set to false. The assertions correctly verify that no fungible token assets are present in the response.clients/js/test/getAssets.test.ts (6)
14-14
: Asset ID updated for consistency.Updated the regularAssetId to use
Hu9vvgNjVDxRo6F8iTEo6sRJikhqoM2zVswR86WAf4C
to maintain consistency with other tests.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
21-26
: Improved asset lookup with find method.The refactoring to use the
find
method to look up assets by ID rather than assuming a specific order improves the test's robustness. This is a better approach as it doesn't rely on the order of results from the API.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
27-72
: Enhanced assertions for asset properties.The test now includes more comprehensive assertions for both compressed and regular assets, including checks for interface, content, compression, grouping, mutable, and burnt properties. This provides better validation of the asset structure.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: 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] 29-29: 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] 30-30: 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] 37-37: 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] 47-47: 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] 52-52: 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] 56-56: 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] 63-63: 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] 66-66: 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] 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] 71-71: 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] 72-72: 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)
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
75-100
: Added test for showUnverifiedCollections option.This test properly verifies that assets can be fetched with the
showUnverifiedCollections
option set to true. It confirms that both assets are returned and verifies the presence of grouping data.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
102-145
: Added test for showCollectionMetadata option.This test appropriately validates that collection metadata is included in the results when
showCollectionMetadata
is set to true. It correctly verifies the structure of the collection metadata.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
147-172
: Added test for showFungible option.This test correctly verifies that fungible token assets are included in the results when
showFungible
is set to true. It properly asserts that a fungible token asset is present in the response.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/src/decorator.ts (11)
15-16
: LGTM: Added new import types for display options enhancement.The import of
GetAssetsRpcInput
andGetAssetRpcInput
correctly supports the new structured input objects.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
23-25
: LGTM: Updated method signature to support display options.The method signature for
getAsset
has been properly updated to accept either the new structured input object or a PublicKey for backward compatibility.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
30-32
: LGTM: Updated method signature to support display options.The method signature for
getAssets
has been properly updated to accept either the new structured input object or an array of PublicKeys for backward compatibility.🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
100-110
: Good refactoring: Pagination validation logic encapsulated in a utility function.Extracting the pagination validation into a dedicated function improves code organization and prevents duplication across methods.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
114-126
: LGTM: Implementation properly handles both new and legacy input formats.The implementation correctly extracts the asset ID and display options from either input format while maintaining backward compatibility.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
127-139
: LGTM: Implementation properly handles both new and legacy input formats.The implementation correctly extracts the asset IDs and display options from either input format while maintaining backward compatibility.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
157-178
: LGTM: Updated to include display options in RPC call.The method correctly passes display options to the RPC call and uses the validatePagination utility function.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
179-199
: LGTM: Updated to include display options in RPC call.The method correctly passes display options to the RPC call and uses the validatePagination utility function.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
200-222
: LGTM: Updated to include display options in RPC call.The method correctly passes display options to the RPC call and uses the validatePagination utility function.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
223-242
: LGTM: Updated to include display options in RPC call.The method correctly passes display options to the RPC call and uses the validatePagination utility function.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
243-280
: LGTM: Updated to include display options and token type in RPC call.The method correctly passes display options and token type to the RPC call and uses the validatePagination utility function.
🧰 Tools
🪛 GitHub Actions: Main
[warning] Code style issues found. Forgot to run Prettier?
clients/js/src/types.ts (11)
7-27
: LGTM: Well-defined DisplayOptions type.The DisplayOptions type is well-structured with proper documentation for each option, including all the requested parameters from the PR objectives:
showUnverifiedCollections
showCollectionMetadata
showFungible
showInscription
35-38
: LGTM: Added display options to GetAssetsByAuthorityRpcInput.Properly added the displayOptions property to the input type with appropriate documentation.
52-55
: LGTM: Added display options to GetAssetsByCreatorRpcInput.Properly added the displayOptions property to the input type with appropriate documentation.
69-72
: LGTM: Added display options to GetAssetsByGroupRpcInput.Properly added the displayOptions property to the input type with appropriate documentation.
81-84
: LGTM: Added display options to GetAssetsByOwnerRpcInput.Properly added the displayOptions property to the input type with appropriate documentation.
158-161
: LGTM: Added tokenType property to SearchAssetsRpcInput.Added a new property to support filtering by token types with appropriate typing.
193-201
: LGTM: Added jsonUri and displayOptions properties to SearchAssetsRpcInput.Properly added both properties with appropriate typing and documentation.
207-217
: LGTM: Created GetAssetRpcInput type.This type properly encapsulates the assetId parameter along with display options for enhanced functionality.
222-232
: LGTM: Created GetAssetsRpcInput type.This type properly encapsulates the assetIds array parameter along with display options for enhanced functionality.
503-509
: LGTM: Enhanced DasApiAssetGrouping with collection details.Added verification status and collection metadata fields to support the new display options functionality.
635-640
: LGTM: Added TokenType with appropriate values.Created a new type with values that support filtering assets by token type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
clients/js/test/getAssets.test.ts (1)
30-74
: 🧹 Nitpick (assertive)Consider using optional chaining instead of non-null assertions
While the non-null assertions are preceded by null checks using
t.truthy
, consider using optional chaining for consistency and better type safety.-t.deepEqual(compressedAsset!.interface, 'V1_NFT'); +t.deepEqual(compressedAsset?.interface, 'V1_NFT');This applies to all non-null assertions in the assertions section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (5)
clients/js/src/decorator.ts
(2 hunks)clients/js/test/getAssets.test.ts
(2 hunks)clients/js/test/getAssetsByCreator.test.ts
(1 hunks)clients/js/test/getAssetsByOwner.test.ts
(1 hunks)clients/js/test/searchAssets.test.ts
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
clients/js/test/getAssetsByOwner.test.ts (2)
clients/js/test/_setup.ts (1)
createUmi
(17-18)clients/js/src/types.ts (1)
DasApiAsset
(241-306)
clients/js/test/getAssetsByCreator.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi
(17-18)
clients/js/test/searchAssets.test.ts (1)
clients/js/test/_setup.ts (1)
createUmi
(17-18)
clients/js/src/decorator.ts (2)
clients/js/src/types.ts (8)
GetAssetRpcInput
(207-217)DasApiAsset
(241-306)GetAssetsRpcInput
(222-232)GetAssetProofRpcResponse
(537-543)GetAssetProofsRpcResponse
(545-548)DasApiAssetList
(355-377)SearchAssetsRpcInput
(87-202)GetAssetSignaturesRpcInput
(550-600)clients/js/src/errors.ts (1)
DasApiError
(1-3)
🔇 Additional comments (17)
clients/js/test/searchAssets.test.ts (2)
116-120
: Remove type assertion toany
Using
as any
bypasses TypeScript's type checking. The correct approach is to update the type definitions to includegroup_definition
as an optional property or use a more specific type assertion.- t.is( - (specificAsset as any).group_definition, - undefined, - 'Expected group_definition to be undefined when showUnverifiedCollections is true' - ); + t.is( + specificAsset.group_definition, + undefined, + 'Expected group_definition to be undefined when showUnverifiedCollections is true' + );
93-148
: Clean implementation of display options testsThe new test cases properly validate the behavior of the
showUnverifiedCollections
parameter with both true and false values. The tests are well-structured with clear assertions that match the expected behavior described in previous discussions.clients/js/test/getAssetsByCreator.test.ts (2)
168-170
: Replace non-null assertion with optional chainingThe non-null assertion (
!
) bypasses TypeScript's type checking. Since you've already verified thatassetWithCollection
exists witht.truthy
, use optional chaining for better type safety.-const collectionGroup = assetWithCollection!.grouping.find( +const collectionGroup = assetWithCollection?.grouping.find( (group) => group.group_key === 'collection' );
27-184
: Well-structured tests for new display optionsThe added tests provide comprehensive coverage of the new parameters and display options. Tests for both verified and unverified creators, as well as different display options like
showUnverifiedCollections
andshowCollectionMetadata
, ensure thorough validation of the API behavior.clients/js/test/getAssetsByOwner.test.ts (2)
96-98
: Replace non-null assertion with optional chainingThe non-null assertion (
!
) bypasses TypeScript's type checking. Since you've already verified thatassetWithCollection
exists witht.truthy
, use optional chaining for better type safety.-const collectionGroup = assetWithCollection!.grouping.find( +const collectionGroup = assetWithCollection?.grouping.find( (group) => group.group_key === 'collection' );
28-179
: Comprehensive test coverage for display optionsThe added tests thoroughly validate the behavior of the new display options, including
showUnverifiedCollections
,showCollectionMetadata
, andshowFungible
. The tests are well-structured with clear assertions for each scenario.clients/js/test/getAssets.test.ts (2)
21-75
: Improved test assertions with asset retrieval by IDThe refactored assertions now find assets by ID rather than relying on array indices, making the tests more robust. The additional null checks and comprehensive property validations strengthen the test coverage.
77-174
: Thorough coverage of new display optionsThe added tests comprehensively validate the behavior of the new display options, including
showUnverifiedCollections
,showCollectionMetadata
, andshowFungible
. The assertions are well-structured and verify the expected behavior for each scenario.clients/js/src/decorator.ts (9)
15-16
: Imports for typed method inputs look good.
The introduction ofGetAssetsRpcInput
andGetAssetRpcInput
helps ensure more structured, self-documenting method parameters.
112-128
: Conditional handling of inputs is correct.
The logic gracefully handles a singlePublicKey
or a typed object withassetId
anddisplayOptions
. No issues found.
129-141
: Consistent approach to handle multiple asset inputs.
This mirrors thegetAsset
logic well and properly throws if assets are not found.
142-149
: Proof retrieval looks straightforward.
No concerns. The code properly throws if no proof is found.
150-158
: Multi-proof retrieval is consistent.
The implementation aligns with the single-proof logic, ensuring uniform error handling.
159-180
: Pagination validation and display options.
The method correctly appliesvalidatePagination
before making the RPC call. PassingdisplayOptions
from the client side is straightforward and type-safe.
181-201
: Creator-based retrieval logic is consistent.
IncludingonlyVerified
is a nice addition that broadens search scenarios without complicating the function signature.
202-224
: No issues with group-based retrieval.
The function uses the same pagination pattern and handles group keys/values well.
225-244
: Owner-based retrieval matches the established interface.
Maintains the same error-handling pattern and optionaldisplayOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (1)
clients/js/src/decorator.ts
(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
clients/js/src/decorator.ts (1)
Learnt from: MarkSackerberg
PR: metaplex-foundation/digital-asset-standard-api#15
File: clients/js/src/decorator.ts:245-282
Timestamp: 2025-04-02T09:23:07.217Z
Learning: The parameter structure in the Digital Asset Standard API is intentionally aligned with "aura" for compatibility reasons, so suggestions to change the parameter structure should be avoided.
🧬 Code Definitions (1)
clients/js/src/decorator.ts (2)
clients/js/src/types.ts (4)
GetAssetRpcInput
(207-217)DasApiAsset
(241-306)GetAssetsRpcInput
(222-232)DasApiAssetList
(355-377)clients/js/src/errors.ts (1)
DasApiError
(1-3)
🔇 Additional comments (11)
clients/js/src/decorator.ts (11)
15-16
: No concerns with newly imported types
The usage ofGetAssetsRpcInput
andGetAssetRpcInput
is correct.
23-25
: Clarify dual usage ofgetAsset
parameter
You might enrich the JSDoc to explain that the method accepts either aPublicKey
or an object with optionaldisplayOptions
, helping users understand when to provide which input type.
30-32
: Likewise, clarifygetAssets
parameter usage
Similar togetAsset
, please consider refining the doc comment to note that callers may pass aPublicKey[]
or a typed object to leveragedisplayOptions
.
99-99
: No concerns
The function signature forcreateDasApiDecorator
is clear.
100-110
: Pagination validation looks good
This check effectively prevents invalid usage of page-based and cursor-based pagination simultaneously.
112-128
: Implementation ofgetAsset
looks correct
Using a type guard to distinguish input object from aPublicKey
is clear. The error handling is consistent.
129-141
: Implementation ofgetAssets
is solid
Handles array-based vs. object-based input gracefully, and error handling is consistent.
142-149
: No concerns ongetAssetProof
The code is straightforward, and error handling is consistent.
150-158
: Looks good
Consistent withgetAssetProof
, ensures we handle multiple assets. No issues found.
245-282
: Implementation is consistent
The large parameter list is aligned with aura's structure as per your note, so no suggestions to reduce it. The code is otherwise fine.
283-307
: Implementation is good
Ensures pagination validation is applied, error handling is consistent, and input is handled appropriately for both asset ID and tree usage.
Adding displayOptions for