feat(api)!: align DAS request types with DAS reference implementation #455
Conversation
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, Default)] | ||
| #[serde(deny_unknown_fields, rename_all = "camelCase")] | ||
| pub struct GetTokenAccounts { | ||
| pub owner_address: Option<String>, |
There was a problem hiding this comment.
Why do we change owner and mint parameters names?
There was a problem hiding this comment.
armyhaylenko
force-pushed
the
feature/MTG-1451-align-das-payloads-with-reference
branch
from
f54d5bb to
d0ed5bb
Compare
armyhaylenko
force-pushed
the
feature/MTG-1451-align-das-payloads-with-reference
branch
from
d0ed5bb to
7327bca
Compare
Summary by CodeRabbit
WalkthroughThe changes refactor API request parameter handling by replacing the types Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
entities/src/api_req_params.rs(9 hunks)integration_tests/src/account_update_tests.rs(2 hunks)integration_tests/src/cnft_tests.rs(18 hunks)nft_ingester/src/api/api_impl.rs(5 hunks)nft_ingester/src/api/dapi/asset.rs(3 hunks)nft_ingester/src/api/dapi/get_asset.rs(2 hunks)nft_ingester/src/api/dapi/get_asset_batch.rs(2 hunks)nft_ingester/src/api/dapi/search_assets.rs(4 hunks)nft_ingester/src/api/util.rs(2 hunks)nft_ingester/tests/api_tests.rs(72 hunks)nft_ingester/tests/bubblegum_tests.rs(2 hunks)nft_ingester/tests/decompress.rs(5 hunks)nft_ingester/tests/dump_tests.rs(7 hunks)postgre-client/src/asset_filter_client.rs(6 hunks)postgre-client/src/storage_traits.rs(2 hunks)postgre-client/tests/asset_filter_client_test.rs(10 hunks)rocks-db/src/clients/asset_client.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
postgre-client/src/storage_traits.rs (1)
postgre-client/src/asset_filter_client.rs (1)
get_grand_total(536-551)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-base-image
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (65)
nft_ingester/tests/dump_tests.rs (7)
7-7: Import change from options to DisplayOptions.The import has been correctly updated to use the new
DisplayOptionsstruct that's replacing the previous options types as part of aligning with the DAS reference implementation.
107-107: Option type replacement in get_asset_pubkeys_filtered call.The code correctly uses the new
DisplayOptionsstruct with theshow_unverified_collectionsflag set to true, maintaining the same behavior as before while following the new type structure.
126-126: Updated import in mtg_441_tests module.The import has been correctly updated to include
DisplayOptionsalong withGetAsset, aligning with the type changes in the API.
212-215: Updated options field in GetAsset for authority_none_collection_authority_some test.The code correctly wraps
DisplayOptionsinSome()and uses the spread operator to default all unspecified fields. This maintains the same behavior while adopting the new type structure.
247-250: Updated options field in GetAsset for authority_some_collection_authority_none test.The options parameter has been properly migrated to the new type, maintaining consistency with the pattern used in other test cases.
281-284: Updated options field in GetAsset for authority_some_collection_authority_some test.The migration to the new type structure has been implemented correctly, preserving the same functionality.
315-318: Updated options field in GetAsset for authority_none_collection_authority_none test.All test cases now consistently use the new
DisplayOptionstype, completing the migration for this test module.nft_ingester/src/api/dapi/get_asset_batch.rs (2)
3-3: Updated import to use DisplayOptions.The import has been correctly changed to use the new unified
DisplayOptionsstruct instead of the previous options type.
25-25:Details
✅ Verification successful
Function parameter type updated from Options to DisplayOptions.
The parameter type has been updated correctly from
OptionstoDisplayOptions, which is a breaking change (as indicated by the "!" in the PR title). This change aligns the function signature with the DAS reference implementation.Verify that all call sites to this function have been updated to use the new parameter type:
🏁 Script executed:
#!/bin/bash # Check for any remaining references to the old parameter type in call sites rg -A 3 -B 3 "get_asset_batch" --type rustLength of output: 8473
Review Update: Call Sites Verified for
DisplayOptionsUsageThe update from
OptionstoDisplayOptionsin theget_asset_batchfunction signature has been successfully verified. Our RG search confirmed that all call sites—in files such asnft_ingester/src/api/builder.rs,nft_ingester/src/api/api_impl.rs, and the integration tests—are now using the updated payload. In particular, the conversion fromGetAssetBatchV0(via.into()) has been correctly applied to construct the newGetAssetBatchparameter withDisplayOptions.No lingering references to the old
Optionstype were found. The breaking change aligns with the DAS reference implementation.nft_ingester/src/api/dapi/get_asset.rs (1)
3-3: Updated import to use DisplayOptions.The import has been correctly changed to use the new unified
DisplayOptionsstruct instead of the previous options type.nft_ingester/tests/decompress.rs (2)
16-16: Updated import to include DisplayOptions.The import has been correctly updated to use the new
DisplayOptionstype fromapi_req_params.
285-288: Updated all GetAsset payload structures to use DisplayOptions.All instances of the options field in the
GetAssetpayload have been correctly updated to useSome(DisplayOptions)with theshow_unverified_collectionsflag set to true and other fields defaulted. This maintains consistent behavior across test cases while adopting the new type.Also applies to: 376-379, 467-470, 558-561
nft_ingester/src/api/util.rs (3)
2-3: Imports updated to use DisplayOptionsThe imports have been updated to replace the previous
GetByMethodsOptionswith the newDisplayOptionstype, aligning with the refactoring effort to use a unified display options type.
9-9: API contract change: get_options() now returns DisplayOptionsThe return type of the
get_options()method in theApiRequesttrait has been changed fromGetByMethodsOptionstoDisplayOptions. This is a breaking change to the API contract that requires all implementers to be updated.
29-30: Implementation updated to handle OptionThe implementation now uses
unwrap_or_default()instead of the previous pattern which likely used.into()on a non-optional type. This properly handles the case whereoptionsmight beNone.integration_tests/src/account_update_tests.rs (2)
4-4: Updated import to use DisplayOptionsThe import statement has been updated to use
DisplayOptionsinstead ofOptions, aligning with the API type changes.
129-129: Updated GetAsset to use OptionThe
optionsfield in theGetAssetrequest is now wrapped inSome()and usesDisplayOptionsinstead of the previous direct usage ofOptions. This change accommodates the new optional options pattern.nft_ingester/tests/bubblegum_tests.rs (2)
10-10: Updated import to use DisplayOptionsThe import statement has been updated to use
DisplayOptionsinstead ofOptions, aligning with the API type changes.
263-266: Updated GetAsset initialization to use OptionThe
optionsfield in theGetAssetpayload is now wrapped inSome()and usesDisplayOptionsinstead of the previous direct usage ofOptions. The functionality remains the same, withshow_unverified_collectionsset totrueand all other fields set to their defaults.nft_ingester/src/api/dapi/asset.rs (3)
4-4: Updated import to use DisplayOptionsThe import statement has been updated to use
DisplayOptionsinstead ofOptions, aligning with the API type changes.
125-125: Updated parameter type to DisplayOptionsThe parameter type for
optionsin theasset_selected_maps_into_full_assetfunction has been changed from&Optionsto&DisplayOptions. This function still expects the same fields to exist in the new type.
164-164: Updated parameter type to DisplayOptionsThe parameter type for
optionsin theget_by_idsfunction has been changed fromOptionstoDisplayOptions. The function logic appears to work with the same fields from the new type.nft_ingester/src/api/dapi/search_assets.rs (4)
4-4: Import updated to use newDisplayOptionstype.The import has been correctly updated to use the new
DisplayOptionstype which replaces the previously used options types as part of the API refactoring.
42-42: Parameter type updated fromGetByMethodsOptionstoDisplayOptions.The function signature has been correctly updated to use the new unified
DisplayOptionstype, aligning with the DAS reference implementation.
114-114: Parameter type updated fromGetByMethodsOptionstoDisplayOptions.The function signature has been correctly updated to use the new unified
DisplayOptionstype, maintaining consistency with the refactoring approach.
173-173: Simplified options passing by removing type conversion.The code has been updated to pass
options.clone()directly instead of the previousoptions.clone().into(). This suggests the conversion is no longer needed since we're now using the same type throughout the call chain.rocks-db/src/clients/asset_client.rs (2)
8-8: Import updated to use newDisplayOptionstype.The import has been correctly updated to use the new
DisplayOptionstype which replaces the previously usedOptionstype.
157-157: Parameter type updated from&Optionsto&DisplayOptions.The function signature has been correctly updated to use the new unified
DisplayOptionstype. The implementation logic and usage of the options object properties remain unchanged, indicating a clean type replacement.postgre-client/tests/asset_filter_client_test.rs (3)
5-5: Import updated to use newDisplayOptionstype.The import has been correctly updated to use the new
DisplayOptionstype which replaces the previously used options types.
59-60: Updated test cases to useDisplayOptionsinstead ofGetByMethodsOptions.All test cases have been consistently updated to use the new
DisplayOptionstype with the same configuration as before, maintaining the functionality while aligning with the refactored API.Also applies to: 128-129, 150-151, 177-178, 199-200, 224-225, 244-245, 304-305
365-370: Updated test case with properDisplayOptionsinitialization.The test case has been updated to use the new
DisplayOptionstype with the appropriate fields set, ensuring consistent behavior with the previous implementation while using the new unified type.integration_tests/src/cnft_tests.rs (5)
3-3: Import updated to use newDisplayOptionstype.The import has been correctly updated to use the new
DisplayOptionstype which replaces the previously usedOptionstype.
18-18: Function parameter type updated fromOptionstoDisplayOptions.The function signature has been correctly updated to use the new unified
DisplayOptionstype, ensuring consistency with the API changes.
27-27: Updated request structure to use optionalDisplayOptions.The
GetAssetstruct construction has been updated to wrap options inSome(), indicating that theoptionsfield in theGetAssetstruct now usesOption<DisplayOptions>instead of directly using the options type. This change allows for explicit optional behavior.
71-71: Updated default options usage in test cases.All test cases have been consistently updated to use
DisplayOptions::default()instead ofOptions::default(), aligning with the refactored API while maintaining the same default behavior.Also applies to: 105-105, 141-141, 250-250, 284-284, 321-321, 354-354, 387-387, 420-420, 452-452, 484-484, 517-517
549-555: Updated complex options initialization in test case.The test case has been updated to use the new
DisplayOptionstype with the appropriate fields set, ensuring consistent behavior with the previous implementation. The..Default::default()pattern is used correctly to set only the necessary fields while using defaults for the rest.nft_ingester/tests/api_tests.rs (6)
109-116: Clean refactoring of SearchAssets options field.The change from direct
SearchAssetsOptionstoSome(DisplayOptions)is consistent across all SearchAssets payloads in test cases, maintaining the existing functionality while aligning with the DAS reference implementation.Also applies to: 142-146, 163-167, 184-188, 199-203, 220-224, 248-252, 270-274, 292-296, 314-318, 337-341, 359-363, 381-385, 403-407, 427-431
553-558: Consistent update of GetAsset options field.All GetAsset payloads now use
Some(DisplayOptions)instead of directly usingOptions, following the unified approach for option handling across API methods.Also applies to: 678-682, 775-779, 897-901, 932-936, 1078-1082, 1090-1094, 1227-1231
1584-1586: Successfully updated GetTokenAccounts options field.The GetTokenAccounts method already had options wrapped in
Option<...>, so the change was limited to usingDisplayOptionsinstead of the previous types. The splitting of owner_address and mint_address into separate lines is a good formatting improvement for readability.Also applies to: 1599-1601, 1614-1616, 1729-1732, 1742-1745, 1755-1758, 1772-1776, 1823-1826, 1836-1839
1869-1873: Properly refactored options in asset query methods.GetAssetsByOwner, GetAssetsByGroup, GetAssetsByCreator, and GetAssetsByAuthority methods all consistently use
Some(DisplayOptions), maintaining API consistency.Also applies to: 1904-1908, 1939-1943, 1974-1978
2008-2011: Updated assertion for refactored struct.Test assertion updated to match the new structure with DisplayOptions, ensuring test validity is maintained through the refactoring.
4794-4794: Properly adjusted assertion to unwrap the now-optional field.The assertion was correctly updated to unwrap the options field before accessing its properties, maintaining test correctness.
nft_ingester/src/api/api_impl.rs (4)
10-13: Imports look good and align with the newDisplayOptionsusage.No immediate issues spotted. Proceed.
332-332: Confirm that using a defaultDisplayOptionsis intended.Falling back to
DisplayOptions::default()may implicitly disable certain features (likeshow_fungibleorshow_inscription). Ensure this behavior is correct.
371-371: Consistent default usage for batch retrieval.Mirrors the single-asset approach. No concerns.
507-516: Field name alignment.Using
owner_address: ownerandmint_address: mintfollows the reference DAS naming. This destructuring is clear and consistent.postgre-client/src/storage_traits.rs (2)
5-5: New import forDisplayOptions.No issues with importing from
api_req_params.
136-137: Grand total method signature extension.Looks consistent with the overall refactoring.
entities/src/api_req_params.rs (10)
116-117: New optionaloptionsinGetAssetsByGroup.Allowing
displayOptionsalias fosters flexible request structures.
132-133: AddedoptionstoGetAssetsByOwner.Ensures consistent usage of
DisplayOptionsacross endpoints.
145-145: Default annotation forcursor.Provides a safe fallback if
cursoris omitted.
154-155: Optional display options inGetAsset.Great improvement for controlling output details on single asset retrieval.
161-162: IntroducedoptionsinGetAssetBatch.Batch retrieval can now leverage display preferences.
189-190: Addedoptionsand defaultcursorforGetAssetsByCreator.This aligns with other endpoints and ensures uniform usage.
202-203:displayOptionsalias inGetAssetsByAuthority.Consistent with other interface changes.
236-237: Owner and mint made optional inGetTokenAccounts.Validation in
get_token_accountsensures at least one is present. Looks fine, but verify you gracefully handle absent owners or mints.
242-244: Allowing customizable display preferences.Enables a broader range of behaviors when retrieving token accounts.
280-281: OptionalDisplayOptionsinSearchAssets.Sufficient for controlling how search results are displayed.
postgre-client/src/asset_filter_client.rs (7)
5-5: LGTM: Import statement updated correctlyThe import has been properly updated to use
DisplayOptionsfromentities::api_req_paramsinstead of the previousGetByMethodsOptions.
35-35: LGTM: Parameter type updated consistentlyFunction parameter type has been correctly updated from
GetByMethodsOptionstoDisplayOptionsto align with the DAS reference implementation.
221-221: LGTM: Parameter type updated consistentlyFunction parameter type has been correctly updated from
GetByMethodsOptionstoDisplayOptions.
236-236: LGTM: Parameter type updated consistentlyFunction parameter type has been correctly updated from
GetByMethodsOptionstoDisplayOptions.
516-516: LGTM: Parameter type updated consistentlyFunction parameter type has been correctly updated from
GetByMethodsOptionstoDisplayOptionsin theAssetPubkeyFilteredFetchertrait implementation.
539-539: LGTM: Parameter type updated consistentlyFunction parameter type has been correctly updated from
GetByMethodsOptionstoDisplayOptionsin theget_grand_totalmethod.
162-167:Details
✅ Verification successful
Verify field compatibility between old and new types
The code continues to use the fields
show_zero_balanceandshow_unverified_collectionsfrom theDisplayOptionsstruct, which suggests these fields exist in both the old and new types with the same semantics.Also applies to: 375-378
🏁 Script executed:
#!/bin/bash # Verify that the DisplayOptions struct contains the same fields used in this file # and check how it differs from the previous GetByMethodsOptions struct # Check the definition of DisplayOptions echo "Checking DisplayOptions definition:" rg -A 15 "struct DisplayOptions" --type rust # Check the definition of the previous GetByMethodsOptions (might no longer exist) echo "Checking GetByMethodsOptions definition (if still exists):" rg -A 15 "struct GetByMethodsOptions" --type rust # Look for any additional methods or default implementations for DisplayOptions echo "Checking for DisplayOptions methods or implementations:" rg "impl.*DisplayOptions" --type rust -A 5Length of output: 1396
Field Compatibility Verified – No Changes Required
After verifying the definitions:
- The new
DisplayOptions(inentities/src/api_req_params.rs) correctly defines bothshow_zero_balanceandshow_unverified_collections.- The previous
GetByMethodsOptionsstruct is no longer present, confirming that the migration toDisplayOptionshas been completed.- The usage in
postgre-client/src/asset_filter_client.rs(lines 162–167 and 375–378) correctly references the updated struct fields.No further modifications are necessary.
No description provided.