Skip to content

Conversation

@lrazovic
Copy link
Member

@lrazovic lrazovic commented May 9, 2025

This pull request introduces several changes aimed at improving code quality, simplifying function calls, and enhancing maintainability across multiple files. The key updates include replacing direct object references with references (&), removing unused constants, and simplifying type constraints in struct definitions. Additionally, some error messages were clarified for better understanding.

Code Quality Improvements:

  • Replaced direct object usage with references in function calls to improve efficiency and align with Rust's borrowing principles. This change is applied across multiple files, including pallets/funding/src/functions/misc.rs, integration-tests/src/tests/otm_edge_cases.rs, and others. [1] [2] [3]

  • Removed unused constants such as USD_DECIMALS from imports in several files, including pallets/funding/src/functions/mod.rs and pallets/funding/src/instantiator/calculations.rs. [1] [2]

Simplifications in Struct Definitions:

  • Simplified type constraints for generic parameters in structs like ProjectMetadata and BiddingTicketSizes by removing the FixedPointNumber constraint where it was unnecessary. This improves flexibility and reduces complexity. [1] [2]

Error Message Enhancements:

  • Clarified error messages for user-facing errors, such as replacing "The amount is too low" with "The amount specified in the call is too low" for better context.

Test Updates:

  • Updated test cases to reflect the use of references in function calls, ensuring consistency with the main codebase. Changes were made in files such as pallets/funding/src/tests/2_evaluation.rs and pallets/funding/src/tests/misc.rs. [1] [2]

Mock and Runtime Adjustments:

  • Modified mock implementations and runtime APIs to align with the new reference-based function signatures, such as in pallets/funding/src/mock.rs.

@lrazovic lrazovic changed the base branch from 05-08-draft_use_plmc_amount_in_evaluation to graphite-base/485 May 9, 2025 15:10
@lrazovic lrazovic force-pushed the graphite-base/485 branch from a1105c5 to 382f61e Compare May 9, 2025 15:10
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 742e30b to 359ca8a Compare May 9, 2025 15:10
@lrazovic lrazovic changed the base branch from graphite-base/485 to 05-08-feat_macros_update_and_documentation May 9, 2025 15:11
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 359ca8a to 15d088b Compare May 9, 2025 15:42
@lrazovic lrazovic force-pushed the 05-08-feat_macros_update_and_documentation branch from 382f61e to 91611a0 Compare May 9, 2025 15:44
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch 2 times, most recently from 67962d0 to a4df891 Compare May 9, 2025 16:52
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch 3 times, most recently from eeada01 to e7ffa1b Compare May 12, 2025 11:36
@lrazovic lrazovic changed the title feat: simplify the acceptable_assets 🎨 Simplify the acceptable_assets May 16, 2025
@lrazovic lrazovic requested a review from Copilot May 16, 2025 14:41
@lrazovic lrazovic self-assigned this May 16, 2025
@lrazovic lrazovic requested a review from dastansam May 16, 2025 14:45
@lrazovic lrazovic marked this pull request as ready for review May 16, 2025 14:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors price conversion and asset management by switching to reference-based API calls, removing unused constants, clarifying error messages, and simplifying type constraints.

  • Use &AssetId in price provider methods and drop the USD_DECIMALS parameter.
  • Remove unnecessary FixedPointNumber bounds and unused imports.
  • Add decimals() and all_ids_and_plmc() to AcceptedFundingAsset, and improve error messaging.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
runtimes/polimec/src/lib.rs Replaced PLMCToAssetBalance with HereToForeignAsset and updated XCM payment API
polimec-common/common/src/lib.rs Updated ProvideAssetPrice trait docs/signature and added convert_back_to_normal_price
polimec-common/common/src/assets.rs Added decimals() and all_ids_and_plmc() to AcceptedFundingAsset
pallets/proxy-bonding/src/mock.rs Changed get_price to accept &Location
pallets/proxy-bonding/src/functions.rs Updated calculate_fee to use the new get_decimals_aware_price signature
pallets/funding/src/types.rs Removed FixedPointNumber constraint from struct generics
pallets/funding/src/tests/runtime_api.rs Adjusted tests to pass references to get_price
pallets/funding/src/tests/misc.rs Aligned test calls with the reference-based price API
pallets/funding/src/tests/3_auction.rs Refactored test to use &Location and the updated price API
pallets/funding/src/tests/2_evaluation.rs Updated test price calls to use references
pallets/funding/src/mock.rs Changed mock get_price to accept &Location
pallets/funding/src/lib.rs Clarified TooLow/TooHigh error messages
pallets/funding/src/instantiator/calculations.rs Simplified price lookup by removing USD_DECIMALS parameter
pallets/funding/src/functions/runtime_api.rs Removed USD_DECIMALS import and updated get_decimals_aware_price usage
pallets/funding/src/functions/mod.rs Cleaned up imports and removed unused constants
pallets/funding/src/functions/misc.rs Switched to reference-based price calls and dropped USD_DECIMALS
pallets/funding/src/functions/2_evaluation.rs Simplified repeated price retrieval calls
integration-tests/src/tests/transaction_payment.rs Updated asset location usage and price API in integration tests
integration-tests/src/tests/otm_edge_cases.rs Refined reference usage and removed USD_DECIMALS in edge-case tests
Cargo.toml Added redundant_clone lint configuration
Comments suppressed due to low confidence (2)

pallets/funding/src/functions/misc.rs:53

  • [nitpick] Consider accepting asset as a reference (&AcceptedFundingAsset) rather than by value to avoid unnecessary copying and align with other APIs that take references.
pub fn calculate_funding_asset_amount(

polimec-common/common/src/assets.rs:60

  • The order of asset IDs in all_ids_and_plmc differs from the original all_ids vector; if ordering matters in downstream code, consider preserving the original sequence or documenting the change.
pub fn all_ids_and_plmc() -> [Location; AcceptedFundingAsset::VARIANT_COUNT + 1] {

let plmc_usd_price = self.execute(|| {
<PriceProviderOf<T>>::get_decimals_aware_price(Location::here(), USD_DECIMALS, PLMC_DECIMALS).unwrap()
});
let plmc_usd_price =
Copy link

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The logic to fetch the PLMC price is repeated in multiple places; extracting this into a helper method (e.g., get_plmc_price()) would reduce duplication and improve clarity.

Copilot uses AI. Check for mistakes.
@lrazovic lrazovic force-pushed the 05-08-feat_macros_update_and_documentation branch from 91611a0 to 6c2a7dd Compare May 19, 2025 06:15
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from e7ffa1b to b5652c4 Compare May 19, 2025 06:15
// 3. Calculate nominal price of PLMC in terms of the target asset.
// Result is in "units of target_asset per unit of PLMC".
// TargetAsset/PLMC = (USD/PLMC) / (USD/TargetAsset)
let price_plmc_in_target_asset_nominal =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this round down a lot of decimals compared to using Perquintill? is it negligible?

@lrazovic lrazovic force-pushed the 05-08-feat_macros_update_and_documentation branch from 6c2a7dd to eb49fc8 Compare May 19, 2025 06:48
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from b5652c4 to 2db9c76 Compare May 19, 2025 06:48
@lrazovic lrazovic force-pushed the 05-08-feat_macros_update_and_documentation branch from eb49fc8 to 5abd90d Compare May 20, 2025 06:21
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 2db9c76 to c9be272 Compare May 20, 2025 06:21
@dastansam dastansam mentioned this pull request May 23, 2025
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 156f866 to e22ac65 Compare May 28, 2025 13:35
@lrazovic lrazovic force-pushed the 05-08-feat_macros_update_and_documentation branch from 5abd90d to 14b0f58 Compare May 28, 2025 13:35
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from e22ac65 to 378a176 Compare May 28, 2025 13:38
@lrazovic lrazovic force-pushed the 05-08-feat_macros_update_and_documentation branch from 14b0f58 to 2fa24d7 Compare May 28, 2025 13:38
@dastansam dastansam force-pushed the 05-08-feat_macros_update_and_documentation branch from 2fa24d7 to 60e9f36 Compare July 9, 2025 06:38
@dastansam dastansam force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 378a176 to 48db973 Compare July 9, 2025 06:38
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 48db973 to d4e10e0 Compare July 25, 2025 14:19
@dastansam dastansam force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from d4e10e0 to 48db973 Compare July 25, 2025 21:21
Copy link
Member Author

lrazovic commented Aug 4, 2025

Merge activity

  • Aug 4, 7:49 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Aug 4, 7:54 AM UTC: Graphite rebased this pull request as part of a merge.
  • Aug 4, 7:55 AM UTC: @lrazovic merged this pull request with Graphite.

@lrazovic lrazovic changed the base branch from 05-08-feat_macros_update_and_documentation to graphite-base/485 August 4, 2025 07:50
@lrazovic lrazovic changed the base branch from graphite-base/485 to main August 4, 2025 07:52
@lrazovic lrazovic force-pushed the 05-09-feat_simplify_the_acceptable_assets branch from 48db973 to be00d51 Compare August 4, 2025 07:53
@lrazovic lrazovic merged commit b16fa7e into main Aug 4, 2025
1 check passed
@lrazovic lrazovic deleted the 05-09-feat_simplify_the_acceptable_assets branch August 4, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants