Claim assets extrinsic #21
Conversation
|
User @franciscoaguirre, please sign the CLA here. |
…=asset-hub-westend --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm
…=westend --target_dir=polkadot --pallet=pallet_xcm
…=coretime-rococo --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_xcm
…=coretime-westend --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_xcm
…=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_xcm
…=bridge-hub-westend --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_xcm
…=collectives-westend --runtime_dir=collectives --target_dir=cumulus --pallet=pallet_xcm
…=people-westend --runtime_dir=people --target_dir=cumulus --pallet=pallet_xcm
…=people-rococo --runtime_dir=people --target_dir=cumulus --pallet=pallet_xcm
…=asset-hub-rococo --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm
Bullrich
force-pushed
the
claim-assets-extrinsic
branch
from
e7ffc40
to
d77dc18
Compare
WalkthroughThe recent updates across various runtimes and the Polkadot ecosystem introduce a new function Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Configuration used: CodeRabbit UI
Files selected for processing (29)
- cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_xcm.rs (14 hunks)
- cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_xcm.rs (15 hunks)
- cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_xcm.rs (13 hunks)
- cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_xcm.rs (13 hunks)
- cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_xcm.rs (13 hunks)
- cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/coretime/coretime-rococo/src/weights/pallet_xcm.rs (12 hunks)
- cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_xcm.rs (17 hunks)
- cumulus/parachains/runtimes/people/people-rococo/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_xcm.rs (5 hunks)
- cumulus/parachains/runtimes/people/people-westend/src/lib.rs (1 hunks)
- cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_xcm.rs (5 hunks)
- polkadot/runtime/rococo/src/lib.rs (7 hunks)
- polkadot/runtime/rococo/src/weights/pallet_xcm.rs (11 hunks)
- polkadot/runtime/westend/src/lib.rs (6 hunks)
- polkadot/runtime/westend/src/weights/pallet_xcm.rs (11 hunks)
- polkadot/xcm/pallet-xcm/src/benchmarking.rs (2 hunks)
- polkadot/xcm/pallet-xcm/src/lib.rs (21 hunks)
- polkadot/xcm/pallet-xcm/src/mock.rs (3 hunks)
- polkadot/xcm/pallet-xcm/src/tests/mod.rs (1 hunks)
- polkadot/xcm/src/lib.rs (4 hunks)
- prdoc/pr_3403.prdoc (1 hunks)
Files not reviewed due to errors (3)
- cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs (Error: unable to parse review)
- cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs (Error: unable to parse review)
- polkadot/xcm/pallet-xcm/src/lib.rs (Error: unable to parse review)
Files skipped from review due to trivial changes (4)
- cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_xcm.rs
- cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_xcm.rs
- cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_xcm.rs
- polkadot/runtime/rococo/src/weights/pallet_xcm.rs
Additional comments: 81
polkadot/xcm/pallet-xcm/src/benchmarking.rs (2)
- 77-83: The addition of the
get_asset()function in theConfigtrait is a good practice for abstracting asset retrieval logic, especially for benchmarking purposes. This approach allows for flexibility and easier maintenance when handling assets in benchmarks.- 338-350: The modifications to the
claim_assetsbenchmark to use theget_asset()function for retrieving an asset for claiming are correctly implemented. This change ensures that the benchmark accurately reflects the process of claiming assets, including the setup of trapping assets for later claiming. It's important to ensure that thedrop_assetsfunction and the context setup in line 346 are correctly aligned with the intended benchmark scenario.cumulus/parachains/runtimes/coretime/coretime-westend/src/weights/pallet_xcm.rs (7)
- 20-20: The metadata at the top of the file, including the date, steps, repeat, and other benchmarking parameters, is correctly updated to reflect the current benchmarking session. This ensures that the weights are up-to-date with the latest codebase and runtime configurations.
- 65-66: The
sendfunction's minimum execution time has been adjusted. It's crucial to ensure that these adjustments are based on recent benchmarking results to maintain accurate transaction cost estimation. If these changes are a result of new benchmarks, this adjustment is appropriate.- 87-88: The
teleport_assetsfunction's execution time has been updated. Similar to thesendfunction, it's important to verify that these changes accurately reflect the latest benchmarking data. Adjustments to execution times directly impact transaction costs and resource allocation.- 62-69: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [65-88]
The execution times for
sendandteleport_assetsfunctions have been adjusted. It's essential to ensure these changes are based on recent benchmarking to maintain accurate transaction cost estimation.
- 129-130: The
force_xcm_versionfunction's minimum execution time has been adjusted. This change should be verified against benchmarking results to ensure it accurately reflects the function's performance.- 140-141: The
force_default_xcm_versionfunction's execution time has been updated. As with other weight adjustments, it's important to validate these changes against benchmarking data.- 334-345: The addition of the
claim_assetsfunction aligns with the PR's objective to facilitate asset claiming. The weight calculation appears to be based on benchmarking data, as indicated by the measured and estimated proof sizes. It's crucial to ensure that the execution time and weight accurately reflect the function's complexity and resource usage.However, it's recommended to verify the benchmarking process for this new function to ensure the weights are accurately calculated.
polkadot/runtime/westend/src/weights/pallet_xcm.rs (5)
- 19-22: The update to the benchmark CLI version, date, and execution details ensures that the weight calculations are based on the latest benchmarking runs. This is crucial for maintaining accurate transaction cost estimation and resource allocation.
- 61-65: The weight calculation for the
sendfunction includes both measured and estimated proof sizes, which is a good practice for ensuring accuracy. However, ensure that the minimum execution time and the weight calculation formula align with the latest benchmarking results and performance expectations.- 81-87: The weight calculation for
teleport_assetsfunction seems to follow a similar pattern assend, including both measured and estimated proof sizes. It's important to verify that these calculations accurately reflect the complexity and resource usage of the function.- 101-107: The
reserve_transfer_assetsfunction's weight calculation also includes detailed proof size summaries. Given the critical nature of asset transfers, it's essential to ensure these weights are meticulously calculated to prevent any potential performance issues.- 339-350: The addition of the
claim_assetsfunction and its weight calculation is a significant change, directly addressing the PR's objective to facilitate users in claiming trapped assets. It's crucial to ensure that the weight calculation accurately reflects the function's resource usage, especially considering its potential impact on user experience.cumulus/parachains/runtimes/coretime/coretime-rococo/src/weights/pallet_xcm.rs (9)
- 19-22: The metadata at the top of the file, including the benchmark CLI version, date, steps, repeat counts, and other details, provides valuable context for understanding the basis of these weight calculations. It's crucial to ensure this metadata is accurate and up-to-date to maintain the credibility and traceability of the benchmarking process.
- 65-66: The update to the minimum execution time for the
sendextrinsic from an unspecified previous value to35_051_000picoseconds appears to be a precise adjustment based on recent benchmarking. It's important to ensure that such adjustments are reflective of actual performance changes observed during benchmarking sessions.- 87-88: The
teleport_assetsextrinsic shows an updated minimum execution time to56_235_000picoseconds. This change should be cross-verified with benchmarking data to confirm its accuracy and to understand the impact of this adjustment on transaction cost estimations.- 129-130: The significant reduction in minimum execution time for the
force_xcm_versionextrinsic to6_226_000picoseconds suggests an optimization or a change in the underlying logic. It's essential to review the corresponding changes in the pallet's logic that led to this improvement.- 140-141: The update to the
force_default_xcm_versionextrinsic, setting its minimum execution time to2_020_000picoseconds, indicates a focus on performance optimization. Verifying the specific changes that contributed to this reduced execution time would be beneficial for understanding the overall impact on the pallet.- 201-202: The
force_suspensionextrinsic's minimum execution time has been updated to1_920_000picoseconds. This change, while seemingly minor, is part of the broader effort to refine weight calculations. Ensuring that these updates are consistent with observed performance metrics is crucial.- 316-317: The
new_queryextrinsic shows an updated minimum execution time to3_363_000picoseconds. This adjustment, along with the proof size summary, should be validated against the latest benchmarking results to ensure it accurately reflects the extrinsic's performance.- 328-329: The update to the
take_responseextrinsic, with a minimum execution time of23_969_000picoseconds, highlights the importance of accurate weight calculations for complex operations. Reviewing the benchmarking methodology that led to this figure would be insightful.- 334-345: The addition of the
claim_assetsextrinsic with a detailed weight calculation, including a minimum execution time of34_071_000picoseconds, is directly related to the PR's objective of facilitating asset claims. This inclusion is a critical component of the PR, and its weight calculation should be thoroughly reviewed to ensure it aligns with the extrinsic's expected performance and resource usage.polkadot/xcm/src/lib.rs (3)
- 515-519: The introduction of the
IdentifyVersiontrait is a strategic enhancement for version management within the XCM framework. It provides a standardized way to retrieve the version of various XCM-related data structures, which is crucial for maintaining compatibility and facilitating upgrades. This change aligns well with the objectives of enhancing interoperability and versioning flexibility in the XCM ecosystem.- 168-176: The implementation of the
IdentifyVersiontrait for the versioned enums starting withV3andV4variants is correctly done. It leverages pattern matching to return the corresponding version constants defined in the respective modules. This approach is both efficient and straightforward, ensuring that the version identification logic is encapsulated within the enums themselves.- 299-308: Similarly, the implementation of the
IdentifyVersiontrait for enums withV2,V3, andV4variants is correctly handled. The use of pattern matching to return the appropriate version constants is consistent with the best practices for Rust enums. This ensures that the version identification logic is clear and maintainable.cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_xcm.rs (2)
- 2-15: The licensing information has been updated to reflect the GNU General Public License version 3. This is a standard procedure for ensuring compliance with open-source licensing requirements.
- 19-23: The benchmarking metadata, including the Substrate benchmark CLI version, date, steps, repeat, range, worst-case map size, hostname, CPU, WASM execution mode, chain, and DB cache size, has been updated. This metadata is crucial for understanding the context and parameters under which the benchmarks were run. It's important to ensure that these details align with the intended benchmarking environment and goals.
cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_xcm.rs (2)
- 2-15: The licensing information has been updated to reflect the GNU General Public License version 3 or later. This is standard for open-source projects and ensures compliance with legal requirements.
- 19-23: The autogenerated file details have been updated, including the Substrate benchmark CLI version to 32.0.0 and the benchmarking date to 2024-02-20. These updates are crucial for ensuring that the weight calculations are based on the latest benchmarking tools and methodologies.
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_xcm.rs (19)
- 19-20: The update to the benchmark CLI version and the benchmarking parameters (steps, repeat, etc.) is noted. Ensure that these changes align with the latest benchmarking standards and practices for Substrate-based runtimes.
- 22-22: The hostname and CPU information have been updated. Verify that this change reflects the actual environment where the benchmarks were run, as this can affect the accuracy of the generated weights.
- 67-68: The minimum execution time for the
sendfunction has been updated. Ensure that this change is based on recent benchmarking results and accurately reflects the function's performance characteristics.- 93-94: The update to the minimum execution time for the
teleport_assetsfunction should be verified against benchmarking data to ensure it accurately represents the function's performance.- 151-152: The minimum execution time for
force_xcm_versionhas been updated. Confirm that this change accurately reflects the function's performance based on the latest benchmarks.- 162-163: The update to the minimum execution time for
force_default_xcm_versionshould be verified to ensure it accurately represents the function's performance.- 189-190: The minimum execution time for
force_subscribe_version_notifyhas been updated. Verify this change against benchmarking data to ensure accuracy.- 215-216: The update to the minimum execution time for
force_unsubscribe_version_notifyshould be verified to ensure it accurately represents the function's performance.- 227-228: The minimum execution time for
force_suspensionhas been updated. Confirm that this change accurately reflects the function's performance based on the latest benchmarks.- 238-239: The minimum execution time for
migrate_supported_versionhas been updated. Verify this change against benchmarking data to ensure accuracy.- 250-251: The minimum execution time for
migrate_version_notifiershas been updated. Confirm that this change accurately reflects the function's performance based on the latest benchmarks.- 262-263: The minimum execution time for
already_notified_targethas been updated. Verify this change against benchmarking data to ensure accuracy.- 285-286: The minimum execution time for
notify_current_targetshas been updated. Confirm that this change accurately reflects the function's performance based on the latest benchmarks.- 297-298: The minimum execution time for
notify_target_migration_failhas been updated. Verify this change against benchmarking data to ensure accuracy.- 308-309: The minimum execution time for
migrate_version_notify_targetshas been updated. Confirm that this change accurately reflects the function's performance based on the latest benchmarks.- 332-333: The minimum execution time for
migrate_and_notify_old_targetshas been updated. Verify this change against benchmarking data to ensure accuracy.- 346-347: The minimum execution time for
new_queryhas been updated. Confirm that this change accurately reflects the function's performance based on the latest benchmarks.- 358-359: The minimum execution time for
take_responsehas been updated. Verify this change against benchmarking data to ensure accuracy.- 367-374: The addition of the
claim_assetsfunction with its weight and execution time is directly related to the PR's objective of facilitating asset claims. Ensure that the execution time and weight accurately reflect the function's complexity and resource usage based on benchmarking data.cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_xcm.rs (21)
- 19-22: The benchmark CLI version and execution environment details have been updated. Ensure that these changes reflect the latest and most accurate benchmarking results for the
pallet_xcmin theasset-hub-westendruntime.- 67-68: The minimum execution time for the
sendfunction has been updated. Verify that this change is consistent with the latest benchmarking results and that it accurately reflects the performance characteristics of thesendextrinsic.- 93-94: The
teleport_assetsfunction's minimum execution time has been significantly increased. This change could impact transaction processing times and fees. Confirm that this adjustment is based on recent benchmarking data and consider its impact on user experience and system performance.- 121-122: The update to the
reserve_transfer_assetsfunction's execution time appears substantial. Ensure that this adjustment is justified by the latest benchmarking and accurately represents the resource consumption of this operation.- 151-152: The
transfer_assetsfunction shows a notable increase in minimum execution time. It's crucial to validate that this change aligns with the latest benchmarking outcomes and does not adversely affect the overall system efficiency.- 161-162: The
executefunction's minimum execution time has been updated. Given its foundational role in executing XCM messages, ensure this change is backed by accurate benchmarking and assess its implications on system throughput.- 171-172: The
force_xcm_versionfunction's execution time has been slightly adjusted. Confirm that this update is based on recent benchmarking and consider its impact on administrative operations within the XCM framework.- 182-183: The
force_default_xcm_versionfunction's execution time has been updated. Ensure this change is reflective of the latest benchmarking data and evaluate its effect on system operations, especially in version management scenarios.- 209-210: The
force_subscribe_version_notifyfunction's execution time has been updated. Verify that this adjustment is based on accurate benchmarking results and assess its implications on version notification mechanisms.- 235-236: The
force_unsubscribe_version_notifyfunction shows an update in execution time. Confirm that this change is justified by the latest benchmarking and does not negatively impact the efficiency of version management operations.- 247-248: The
force_suspensionfunction's execution time has been updated. Given its potential impact on XCM execution, ensure this change is based on recent benchmarking and assess its implications on system resilience.- 252-261: The
migrate_supported_versionfunction has been introduced with specific weights and execution times. Ensure that this new function is thoroughly benchmarked and that its implementation aligns with the overall objectives of version management within the XCM framework.- 264-273: The
migrate_version_notifiersfunction has been added, indicating an update in version notification mechanisms. Verify that this addition is backed by comprehensive benchmarking and assess its compatibility with existing version management practices.- 276-285: The
already_notified_targetfunction's execution time has been updated. Confirm that this adjustment accurately reflects the performance characteristics of version notification mechanisms and is based on the latest benchmarking data.- 305-306: The
notify_current_targetsfunction's execution time has been updated. Ensure this change is justified by recent benchmarking and evaluate its impact on the efficiency of version notification processes.- 311-320: The
notify_target_migration_failfunction's execution time has been updated. Verify that this adjustment is based on accurate benchmarking and assess its implications on the robustness of version migration mechanisms.- 322-331: The
migrate_version_notify_targetsfunction has been introduced with specific weights and execution times. Confirm that this new function is thoroughly benchmarked and assess its role in the overall version notification and migration strategy.- 347-358: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [334-355]
The
migrate_and_notify_old_targetsfunction has been added, indicating a comprehensive approach to version migration and notification. Ensure that this function is accurately benchmarked and that its implementation supports the objectives of efficient version management.
- 366-367: The
new_queryfunction's execution time has been updated. Given its role in managing XCM queries, confirm that this change is based on the latest benchmarking and evaluate its impact on query processing efficiency.- 378-379: The
take_responsefunction's execution time has been significantly updated. Ensure that this adjustment accurately reflects the resource consumption of processing XCM query responses and is based on recent benchmarking data.- 386-394: The introduction of the
claim_assetsfunction with specific weights and execution times is a critical addition to thepallet_xcm. Confirm that this function is thoroughly benchmarked, and its implementation aligns with the PR objectives of facilitating asset claims for users.polkadot/xcm/pallet-xcm/src/mock.rs (3)
- 175-175: The error message "Intentional send failure used in tests" is clear and indicates the purpose of the failure. However, it's essential to ensure that this behavior is documented in the test cases that rely on this functionality to avoid confusion for future maintainers.
- 224-224: Returning
SendError::NotApplicableinTestPaidForPara3000SendXcmwhen the destination is notPara3000or isNoneis logical. However, it's crucial to document the specific use case ofTestPaidForPara3000SendXcmto clarify why it only allows sending toPara3000. This helps maintainers understand the context and limitations of this mock sender.Also applies to: 227-227
- 636-638: The
get_asset()function provides a straightforward way to create a test asset with the existential deposit as its fungible amount. This is a useful addition for testing purposes. However, ensure that the existential deposit value used here aligns with the test scenarios' requirements, as different tests might require assets with varying amounts.cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs (1)
- 751-756: The implementation of
get_asset()function correctly returns anAssetstruct with hardcoded values foridandfun. This function is straightforward and serves its purpose within the context of the runtime API. However, it's important to ensure that the hardcodedEXISTENTIAL_DEPOSITvalue aligns with the intended asset properties and that usingLocation::parent()as the asset ID fits the use case correctly. If theEXISTENTIAL_DEPOSITvalue or asset location might vary based on runtime conditions, consider parameterizing these values or providing a mechanism to retrieve them dynamically.cumulus/parachains/runtimes/people/people-rococo/src/lib.rs (1)
- 707-713: The implementation of
get_asset()correctly returns anAssetwith theidset to the parent location andfunset to a fungible amount equal to theExistentialDeposit. This aligns with the PR's objective to facilitate asset claiming by providing a standardized way to identify assets across different parachains and runtimes.
- Correctness: The function correctly constructs an
Assetobject with appropriate values.- Logic: The logic to use
Location::parent()andExistentialDeposit::get()is sound, ensuring the asset is identified relative to the parent chain and uses the existential deposit as the fungible amount, which is a sensible default for asset identification.- Performance: Given the simplicity of the function and its return of a static value, there are no immediate performance concerns.
- Best Practices: The function adheres to Rust and Substrate best practices in terms of syntax and structure.
Overall, the addition of
get_asset()is well-implemented and serves its intended purpose effectively.cumulus/parachains/runtimes/people/people-westend/src/lib.rs (1)
- 707-713: The implementation of the
get_asset()function within theimpl_runtime_apis!block correctly returns anAssetwith a parent location and a fungible existential deposit. This aligns with the PR's objective to facilitate asset claiming by providing a standardized way to identify assets across different parachains and runtimes. The use ofExistentialDeposit::get()ensures that the amount is dynamically fetched based on the runtime's configuration, which is a good practice for maintainability and flexibility.However, it's important to ensure that the
AssetIdandFungibletypes, along with theLocation::parent()function, are correctly imported and available in this context. Additionally, the existential deposit value should be verified to ensure it's appropriate for the assets being claimed. If the existential deposit is too low or too high, it might not serve the intended purpose effectively.Overall, the addition seems well-integrated with the existing runtime configuration and follows Substrate development practices. Just ensure that all dependencies are correctly managed and that the existential deposit value is carefully considered.
cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs (1)
- 728-734: The implementation of the
get_assetfunction correctly constructs anAssetstruct with anidderived fromLocation::parent()and afun(fungibility) set to the existential deposit. This aligns with the PR's objective to facilitate asset claiming by providing a standardized way to identify assets across different parachains and runtimes.However, it's important to ensure that the existential deposit (
ExistentialDeposit::get()) used here matches the intended value for the asset in question. If the existential deposit is not correctly set or retrieved, it could lead to discrepancies in asset identification or claiming processes.Additionally, consider adding documentation comments to the
get_assetfunction to explain its purpose, parameters, and return value. This will improve code maintainability and readability, especially for developers who might work on this codebase in the future.polkadot/xcm/pallet-xcm/src/tests/mod.rs (1)
- 471-519: The new test function
claim_assets_workscorrectly simulates the process of trapping and then claiming assets using theclaim_assetsextrinsic. The test follows a logical sequence of actions to ensure the functionality works as expected. However, there are a few areas that could be improved for clarity and maintainability:
Comments and Documentation: Adding comments to explain the purpose of each major step within the test could improve readability and maintainability. This is particularly useful for complex tests involving multiple steps and assertions.
Error Handling: While the test checks for successful operations (
assert_ok!), it might be beneficial to also include scenarios where operations fail due to incorrect parameters or states. This ensures the error handling paths are also tested.Use of Constants: The test uses hardcoded values for balances and amounts (
INITIAL_BALANCE,SEND_AMOUNT). While this is acceptable, ensuring these values are defined in a consistent manner across tests (if they are reused) or documenting the choice of values could improve the test's readability and ease of maintenance.Asset Verification: After claiming the assets, the test verifies the balance restoration and that the asset traps are cleared. Including additional checks to verify the state of the assets (e.g., ownership, location) post-claim could strengthen the test's validation of the
claim_assetsfunctionality.Overall, the test is well-structured and achieves its purpose of validating the
claim_assetsextrinsic's functionality. Enhancing the test with the above suggestions could further improve its quality and robustness.However, consider the above points for refining the test further.
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs (1)
- 841-846: The implementation of the
get_asset()function correctly constructs anAssetstruct with anidderived fromLocation::parent()and afun(fungibility) set to the existential deposit. This approach is consistent with the typical pattern for defining assets in the Polkadot ecosystem. However, it's important to ensure that the existential deposit value used here (ExistentialDeposit::get()) is correctly configured for the asset in question, as this value directly impacts the usability and security of the asset within the network.cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs (1)
- 1439-1444: The new function
get_asset()within theimpl_runtime_apis!block correctly returns anAssetwith the parent location and a fungible existential deposit amount. This implementation aligns with the expected behavior for a function that retrieves a basic asset configuration. However, it's important to ensure that theExistentialDepositvalue used here is correctly configured and represents a meaningful amount for the asset in question. Additionally, consider adding documentation comments to this function to explain its purpose, especially since it's part of the runtime API, which could be used by external tools or services.polkadot/runtime/rococo/src/lib.rs (1)
- 2470-2470: The conditional compilation for running migration tests checks an environment variable
RUN_MIGRATION_TESTS. This is a good practice for selectively enabling tests. However, it's crucial to document this clearly in the test code or related documentation to ensure that future developers or CI environments are aware of how to enable these tests.Ensure to document the usage of
RUN_MIGRATION_TESTSenvironment variable for enabling migration tests.
|
|
||
| fn get_asset() -> Asset { | ||
| Asset { | ||
| id: AssetId(Location::parent()), | ||
| fun: Fungible(ExistentialDeposit::get()), | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of get_asset() correctly constructs an Asset with a parent location and a fungible existential deposit. However, it's important to ensure that ExistentialDeposit::get() is correctly retrieving the value intended for use in this context. Additionally, consider adding documentation to this function to explain its purpose, especially since it plays a critical role in asset management within the XCM context.
/// Retrieves the default asset used within the parachain.
/// This asset is identified by its location (parent) and its fungibility based on the existential deposit.
fn get_asset() -> Asset {
Asset {
id: AssetId(Location::parent()),
fun: Fungible(ExistentialDeposit::get()),
}
}| /// Proof: `PolkadotXcm::SupportedVersion` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::VersionDiscoveryQueue` (r:1 w:1) | ||
| /// Proof: `PolkadotXcm::VersionDiscoveryQueue` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::SafeXcmVersion` (r:1 w:0) | ||
| /// Proof: `PolkadotXcm::SafeXcmVersion` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::HostConfiguration` (r:1 w:0) | ||
| /// Proof: `ParachainSystem::HostConfiguration` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::PendingUpwardMessages` (r:1 w:1) | ||
| /// Proof: `ParachainSystem::PendingUpwardMessages` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| fn notify_current_targets() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `106` | ||
| // Estimated: `6046` | ||
| // Minimum execution time: 28_025_000 picoseconds. | ||
| Weight::from_parts(28_524_000, 0) | ||
| // Minimum execution time: 23_817_000 picoseconds. | ||
| Weight::from_parts(24_520_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 6046)) | ||
| .saturating_add(T::DbWeight::get().reads(7)) | ||
| .saturating_add(T::DbWeight::get().writes(3)) | ||
| } | ||
| /// Storage: PolkadotXcm VersionNotifyTargets (r:3 w:0) | ||
| /// Proof Skipped: PolkadotXcm VersionNotifyTargets (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: `PolkadotXcm::VersionNotifyTargets` (r:4 w:0) | ||
| /// Proof: `PolkadotXcm::VersionNotifyTargets` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| fn notify_target_migration_fail() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `136` | ||
| // Estimated: `8551` | ||
| // Minimum execution time: 8_166_000 picoseconds. | ||
| Weight::from_parts(8_314_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 8551)) | ||
| .saturating_add(T::DbWeight::get().reads(3)) | ||
| // Estimated: `11026` | ||
| // Minimum execution time: 11_042_000 picoseconds. | ||
| Weight::from_parts(11_578_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 11026)) | ||
| .saturating_add(T::DbWeight::get().reads(4)) | ||
| } | ||
| /// Storage: PolkadotXcm VersionNotifyTargets (r:4 w:2) | ||
| /// Proof Skipped: PolkadotXcm VersionNotifyTargets (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: `PolkadotXcm::VersionNotifyTargets` (r:5 w:2) | ||
| /// Proof: `PolkadotXcm::VersionNotifyTargets` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| fn migrate_version_notify_targets() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `106` | ||
| // Estimated: `10996` | ||
| // Minimum execution time: 14_871_000 picoseconds. | ||
| Weight::from_parts(15_374_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 10996)) | ||
| .saturating_add(T::DbWeight::get().reads(4)) | ||
| // Measured: `100` | ||
| // Estimated: `13465` | ||
| // Minimum execution time: 17_306_000 picoseconds. | ||
| Weight::from_parts(17_817_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 13465)) | ||
| .saturating_add(T::DbWeight::get().reads(5)) | ||
| .saturating_add(T::DbWeight::get().writes(2)) | ||
| } | ||
| /// Storage: PolkadotXcm VersionNotifyTargets (r:4 w:2) | ||
| /// Proof Skipped: PolkadotXcm VersionNotifyTargets (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: PolkadotXcm SupportedVersion (r:1 w:0) | ||
| /// Proof Skipped: PolkadotXcm SupportedVersion (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: PolkadotXcm VersionDiscoveryQueue (r:1 w:1) | ||
| /// Proof Skipped: PolkadotXcm VersionDiscoveryQueue (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: PolkadotXcm SafeXcmVersion (r:1 w:0) | ||
| /// Proof Skipped: PolkadotXcm SafeXcmVersion (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: ParachainSystem HostConfiguration (r:1 w:0) | ||
| /// Proof Skipped: ParachainSystem HostConfiguration (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: ParachainSystem PendingUpwardMessages (r:1 w:1) | ||
| /// Proof Skipped: ParachainSystem PendingUpwardMessages (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: `PolkadotXcm::VersionNotifyTargets` (r:5 w:2) | ||
| /// Proof: `PolkadotXcm::VersionNotifyTargets` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::SupportedVersion` (r:1 w:0) | ||
| /// Proof: `PolkadotXcm::SupportedVersion` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::VersionDiscoveryQueue` (r:1 w:1) | ||
| /// Proof: `PolkadotXcm::VersionDiscoveryQueue` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::SafeXcmVersion` (r:1 w:0) | ||
| /// Proof: `PolkadotXcm::SafeXcmVersion` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::HostConfiguration` (r:1 w:0) | ||
| /// Proof: `ParachainSystem::HostConfiguration` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::PendingUpwardMessages` (r:1 w:1) | ||
| /// Proof: `ParachainSystem::PendingUpwardMessages` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| fn migrate_and_notify_old_targets() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `112` | ||
| // Estimated: `11002` | ||
| // Minimum execution time: 33_611_000 picoseconds. | ||
| Weight::from_parts(34_008_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 11002)) | ||
| .saturating_add(T::DbWeight::get().reads(9)) | ||
| // Measured: `106` | ||
| // Estimated: `13471` | ||
| // Minimum execution time: 32_141_000 picoseconds. | ||
| Weight::from_parts(32_954_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 13471)) | ||
| .saturating_add(T::DbWeight::get().reads(10)) | ||
| .saturating_add(T::DbWeight::get().writes(4)) | ||
| } | ||
| /// Storage: `PolkadotXcm::QueryCounter` (r:1 w:1) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [51-342]
The weight calculations for various XCM-related functions (send, teleport_assets, reserve_transfer_assets, transfer_assets, execute, force_xcm_version, force_default_xcm_version, force_subscribe_version_notify, force_unsubscribe_version_notify, force_suspension, migrate_supported_version, migrate_version_notifiers, already_notified_target, notify_current_targets, notify_target_migration_fail, migrate_version_notify_targets, migrate_and_notify_old_targets, new_query, take_response, claim_assets) have been updated. These updates are critical for ensuring accurate transaction cost estimation and resource allocation during XCM operations. It's essential to verify that these weight calculations are based on recent and relevant benchmarks to maintain the efficiency and security of the network. The presence of extremely large minimum execution times (e.g., 18_446_744_073_709_551_000 picoseconds) for some functions (reserve_transfer_assets, transfer_assets, execute) seems to be an anomaly and warrants further investigation.
- Weight::from_parts(18_446_744_073_709_551_000, 0)
+ // Please verify if the extremely large weights are intentional or the result of a benchmarking error.
+ // If it's an error, replace with the correct weight based on updated benchmarks.| doc: | ||
| - audience: Runtime User | ||
| description: | | ||
| There's a new extrinsic in `pallet-xcm` for claiming assets. | ||
| This means that if your assets ever get trapped while teleporting or doing reserve asset transfers, | ||
| you can easily claim them by calling this new extrinsic. | ||
| - audience: Runtime Dev | ||
| description: | | ||
| There's a new extrinsic in `pallet-xcm` that needs a new configuration item for its benchmarks. | ||
| It's a simple function in `pallet_xcm::benchmarking::Config`, `get_asset`, that returns a valid asset | ||
| handled by the AssetTransactor of the chain. | ||
| If you're already using `pallet-xcm-benchmarks`, then you already have this function there and can | ||
| just copy and paste it. |
There was a problem hiding this comment.
The PRDoc provides a clear overview of the new claim_assets extrinsic for both runtime users and developers. However, it would be beneficial to include technical details or example usage for developers to better understand how to implement or interact with the new extrinsic in their projects.
| fn get_asset() -> Asset { | ||
| Asset { | ||
| id: AssetId(Location::parent()), | ||
| fun: Fungible(ExistentialDeposit::get()), | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of get_asset() function correctly constructs an Asset struct with a parent location and a fungible existential deposit. However, there are a few considerations and improvements to discuss:
-
Hardcoded Values: The function uses
Location::parent()andExistentialDeposit::get()to set theidandfunfields of theAssetstruct, respectively. While this is acceptable for specific use cases, consider if these values need to be configurable or if they should adapt based on different runtime conditions or configurations. -
Documentation: There's no documentation comment for the
get_asset()function. Adding a brief doc comment explaining the purpose of the function, the meaning of the returnedAsset, and any relevant usage notes would improve code readability and maintainability. -
Error Handling: The current implementation assumes that
ExistentialDeposit::get()will always succeed. If there's any scenario where fetching the existential deposit could fail or if the value might not be set as expected, consider adding error handling or validation logic. -
Performance Considerations: Given the simplicity of the function and its reliance on constant or configuration values, there are no immediate performance concerns. However, if the logic becomes more complex in the future, keep an eye on potential performance implications.
-
Testing: Ensure that there are tests covering the
get_asset()function, particularly if the logic becomes more complex or if it starts depending on runtime-specific configurations. Tests should validate that the function returns the correctAssetstruct under various conditions.
Consider adding documentation for the get_asset() function and evaluating if the hardcoded values meet all expected use cases. Additionally, ensure that appropriate tests are in place.
| /// Proof: `PolkadotXcm::SupportedVersion` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::VersionDiscoveryQueue` (r:1 w:1) | ||
| /// Proof: `PolkadotXcm::VersionDiscoveryQueue` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::SafeXcmVersion` (r:1 w:0) | ||
| /// Proof: `PolkadotXcm::SafeXcmVersion` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::HostConfiguration` (r:1 w:0) | ||
| /// Proof: `ParachainSystem::HostConfiguration` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::PendingUpwardMessages` (r:1 w:1) | ||
| /// Proof: `ParachainSystem::PendingUpwardMessages` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| fn notify_current_targets() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `106` | ||
| // Estimated: `6046` | ||
| // Minimum execution time: 27_840_000 picoseconds. | ||
| Weight::from_parts(28_248_000, 0) | ||
| // Minimum execution time: 23_577_000 picoseconds. | ||
| Weight::from_parts(24_324_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 6046)) | ||
| .saturating_add(T::DbWeight::get().reads(7)) | ||
| .saturating_add(T::DbWeight::get().writes(3)) | ||
| } | ||
| /// Storage: PolkadotXcm VersionNotifyTargets (r:3 w:0) | ||
| /// Proof Skipped: PolkadotXcm VersionNotifyTargets (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: `PolkadotXcm::VersionNotifyTargets` (r:4 w:0) | ||
| /// Proof: `PolkadotXcm::VersionNotifyTargets` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| fn notify_target_migration_fail() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `136` | ||
| // Estimated: `8551` | ||
| // Minimum execution time: 8_245_000 picoseconds. | ||
| Weight::from_parts(8_523_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 8551)) | ||
| .saturating_add(T::DbWeight::get().reads(3)) | ||
| // Estimated: `11026` | ||
| // Minimum execution time: 11_014_000 picoseconds. | ||
| Weight::from_parts(11_223_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 11026)) | ||
| .saturating_add(T::DbWeight::get().reads(4)) | ||
| } | ||
| /// Storage: PolkadotXcm VersionNotifyTargets (r:4 w:2) | ||
| /// Proof Skipped: PolkadotXcm VersionNotifyTargets (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: `PolkadotXcm::VersionNotifyTargets` (r:5 w:2) | ||
| /// Proof: `PolkadotXcm::VersionNotifyTargets` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| fn migrate_version_notify_targets() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `106` | ||
| // Estimated: `10996` | ||
| // Minimum execution time: 14_780_000 picoseconds. | ||
| Weight::from_parts(15_173_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 10996)) | ||
| .saturating_add(T::DbWeight::get().reads(4)) | ||
| // Measured: `100` | ||
| // Estimated: `13465` | ||
| // Minimum execution time: 16_887_000 picoseconds. | ||
| Weight::from_parts(17_361_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 13465)) | ||
| .saturating_add(T::DbWeight::get().reads(5)) | ||
| .saturating_add(T::DbWeight::get().writes(2)) | ||
| } | ||
| /// Storage: PolkadotXcm VersionNotifyTargets (r:4 w:2) | ||
| /// Proof Skipped: PolkadotXcm VersionNotifyTargets (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: PolkadotXcm SupportedVersion (r:1 w:0) | ||
| /// Proof Skipped: PolkadotXcm SupportedVersion (max_values: None, max_size: None, mode: Measured) | ||
| /// Storage: PolkadotXcm VersionDiscoveryQueue (r:1 w:1) | ||
| /// Proof Skipped: PolkadotXcm VersionDiscoveryQueue (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: PolkadotXcm SafeXcmVersion (r:1 w:0) | ||
| /// Proof Skipped: PolkadotXcm SafeXcmVersion (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: ParachainSystem HostConfiguration (r:1 w:0) | ||
| /// Proof Skipped: ParachainSystem HostConfiguration (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: ParachainSystem PendingUpwardMessages (r:1 w:1) | ||
| /// Proof Skipped: ParachainSystem PendingUpwardMessages (max_values: Some(1), max_size: None, mode: Measured) | ||
| /// Storage: `PolkadotXcm::VersionNotifyTargets` (r:5 w:2) | ||
| /// Proof: `PolkadotXcm::VersionNotifyTargets` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::SupportedVersion` (r:1 w:0) | ||
| /// Proof: `PolkadotXcm::SupportedVersion` (`max_values`: None, `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::VersionDiscoveryQueue` (r:1 w:1) | ||
| /// Proof: `PolkadotXcm::VersionDiscoveryQueue` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `PolkadotXcm::SafeXcmVersion` (r:1 w:0) | ||
| /// Proof: `PolkadotXcm::SafeXcmVersion` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::HostConfiguration` (r:1 w:0) | ||
| /// Proof: `ParachainSystem::HostConfiguration` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| /// Storage: `ParachainSystem::PendingUpwardMessages` (r:1 w:1) | ||
| /// Proof: `ParachainSystem::PendingUpwardMessages` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) | ||
| fn migrate_and_notify_old_targets() -> Weight { | ||
| // Proof Size summary in bytes: | ||
| // Measured: `112` | ||
| // Estimated: `11002` | ||
| // Minimum execution time: 33_422_000 picoseconds. | ||
| Weight::from_parts(34_076_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 11002)) | ||
| .saturating_add(T::DbWeight::get().reads(9)) | ||
| // Measured: `106` | ||
| // Estimated: `13471` | ||
| // Minimum execution time: 31_705_000 picoseconds. | ||
| Weight::from_parts(32_166_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 13471)) | ||
| .saturating_add(T::DbWeight::get().reads(10)) | ||
| .saturating_add(T::DbWeight::get().writes(4)) | ||
| } | ||
| /// Storage: `PolkadotXcm::QueryCounter` (r:1 w:1) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [51-342]
The weight calculations for various XCM operations have been adjusted. These adjustments are based on execution times, proof sizes, and storage operations. It's important to ensure that these weight calculations accurately reflect the computational and storage resources required for each operation to maintain network efficiency and prevent spam.
However, there are some concerns regarding the reserve_transfer_assets and transfer_assets functions (lines 95-102 and 104-111, respectively), which have a minimum execution time set to 18_446_744_073_709_551_000 picoseconds. This value seems to be an overflow or an error in the benchmarking process, as it translates to an impractically high number of years. Similar issues are observed with the execute function (lines 113-120). These anomalies could significantly impact the network's operation if not addressed.
- Weight::from_parts(18_446_744_073_709_551_000, 0)
+ // Corrected weight calculation needed herePlease review the benchmarking process for these functions to ensure the weights are calculated correctly and update the values accordingly.
| return None; | ||
| } | ||
| // Lease not yet started, ignore: | ||
| if lease.iter().any(Option::is_none) { | ||
| return None | ||
| return None; |
There was a problem hiding this comment.
The code attempts to return None for a parachain lease if the lease has not started yet. However, the condition lease.iter().any(Option::is_none) seems incorrect for this purpose. This condition checks if any element in the lease vector is None, which does not directly correlate to whether the lease has started. A more accurate approach might involve checking the lease start and end times against the current block number.
Consider revising the logic to accurately determine if the lease has not started based on lease periods and the current block number.
|
|
||
| fn get_asset() -> Asset { | ||
| Asset { | ||
| id: AssetId(Location::here()), | ||
| fun: Fungible(ExistentialDeposit::get()), | ||
| } | ||
| } |
There was a problem hiding this comment.
The function get_asset() within the pallet_xcm::benchmarking::Config implementation returns a hard-coded asset with an ExistentialDeposit amount. This approach is straightforward but lacks flexibility for different testing scenarios where varying asset characteristics might be needed.
To enhance flexibility for benchmarking different scenarios, consider allowing the asset characteristics (e.g., amount) to be configurable or passed as parameters to the get_asset() function.
fn get_asset(amount: Balance) -> Asset {
Asset {
id: AssetId(Location::here()),
fun: Fungible(amount),
}
}| @@ -2544,7 +2551,7 @@ | |||
| #[tokio::test] | |||
| async fn run_migrations() { | |||
| if var("RUN_MIGRATION_TESTS").is_err() { | |||
| return | |||
| return; | |||
There was a problem hiding this comment.
The remote test setup for running migrations uses environment variables to configure the WebSocket transport and state snapshot. This setup is practical for flexibility in testing environments. However, there's a hardcoded return statement that prevents the test from running unless a specific environment variable is set.
This approach might unintentionally skip important migration tests in automated environments if the environment variable is not correctly set or overlooked.
Consider implementing a more explicit control mechanism for enabling or disabling tests, possibly through CI/CD pipeline configurations or a dedicated test configuration file.
| return None; | ||
| } | ||
| // Lease not yet started, ignore: | ||
| if lease.iter().any(Option::is_none) { | ||
| return None | ||
| return None; |
There was a problem hiding this comment.
The migration logic for GetLegacyLeaseImpl checks for empty leases and leases that have not yet started. However, it does not account for leases that may have already ended. Consider adding logic to handle expired leases appropriately to ensure the migration process is accurate and comprehensive.
// Suggested check for expired leases
if lease.iter().any(|lease_end| lease_end <= ¤t_block) {
return None;
}|
|
||
| fn get_asset() -> Asset { | ||
| Asset { | ||
| id: AssetId(Location::here()), | ||
| fun: Fungible(ExistentialDeposit::get()), | ||
| } | ||
| } |
There was a problem hiding this comment.
The get_asset() function in the pallet_xcm_benchmarks::generic::Config implementation returns a hardcoded asset with an ExistentialDeposit amount. This approach might not be flexible for different testing scenarios or future changes to the existential deposit. Consider parameterizing this function or providing a way to configure the asset details externally.
// Example refinement to allow external configuration
fn get_asset() -> Asset {
// Fetch asset details from a configurable source or parameter
}
… import message (paritytech#4021) Sometimes you need to debug some issues just by the logs and reconstruct what happened. In these scenarios it would be nice to know if a block was imported as best block, and what it parent was. So here I propose to change the output of the informant to this: ``` 2024-04-05 20:38:22.004 INFO ⋮substrate: [Parachain] ✨ Imported #18 (0xe7b3…4555 -> 0xbd6f…ced7) 2024-04-05 20:38:24.005 INFO ⋮substrate: [Parachain] ✨ Imported #19 (0xbd6f…ced7 -> 0x4dd0…d81f) 2024-04-05 20:38:24.011 INFO ⋮substrate: [jobless-children-5352] 🌟 Imported #42 (0xed2e…27fc -> 0x718f…f30e) 2024-04-05 20:38:26.005 INFO ⋮substrate: [Parachain] ✨ Imported #20 (0x4dd0…d81f -> 0x6e85…e2b8) 2024-04-05 20:38:28.004 INFO ⋮substrate: [Parachain] 🌟 Imported #21 (0x6e85…e2b8 -> 0xad53…2a97) 2024-04-05 20:38:30.004 INFO ⋮substrate: [Parachain] 🌟 Imported #22 (0xad53…2a97 -> 0xa874…890f) ``` --------- Co-authored-by: Bastian Köcher <[email protected]>
* Update dependencies Upgrades Substrate based dependencies from v2.0.0 -> v2.0.0-alpha.1 and uses the `jsonrpsee`'s new feature flags. The actual code hasn't been updated though, so this won't compile. * Use `RawClient`s from `jsonrpsee` * Update to use jsonrpsee's new API * Hook up Ethereum Bridge Runtime, Relay, and Node Runtime * Bump `parity-crypto` from v0.4 to v0.6 Fixes error when trying to compile tests. This was caused by `parity-crypto` v0.4's use of `parity-secp256k1` over `secp256k1'. Using the Parity fork meant multiple version of the same underlying C library were being pulled in. `parity-crypto` v0.6 moved away from this, only relying on `secp256k1` thus fixing the issue.
If an XCM execution fails or ends with leftover assets, these will be trapped.
In order to claim them, a custom XCM has to be executed, with the
ClaimAssetinstruction.However, arbitrary XCM execution is not allowed everywhere yet and XCM itself is still not easy enough to use for users out there with trapped assets.
This new extrinsic in
pallet-xcmwill allow these users to easily claim their assets, without concerning themselves with writing arbitrary XCMs.Copy of paritytech#3403
Summary by CodeRabbit
New Features
get_assetfunction across various runtimes and pallets for retrieving assets with specific properties.claim_assetsextrinsic to thepallet_xcmfor claiming trapped assets, enhancing asset management capabilities.IdentifyVersiontrait to facilitate version identification based on enum variants, improving system interoperability and version control.Bug Fixes
returnstatements and adjusting vector initialization in various runtimes.Performance Improvements
pallet_xcmacross multiple runtimes, reflecting optimizations in function execution times, proof sizes, and storage operations.Documentation
claim_assetsextrinsic topallet_xcm.Tests
claim_assetsextrinsic, ensuring proper asset claiming and balance restoration.