Skip to content

Conversation

@lrazovic
Copy link
Member

@lrazovic lrazovic commented May 13, 2025

This pull request simplifies the create_bucket_from_metadata function and its usage across the pallets/funding module by removing unnecessary error handling and making the Bucket::new constructor more concise. Additionally, it includes minor cleanup in related files.

Simplification of create_bucket_from_metadata:

  • pallets/funding/src/functions/misc.rs: Removed the Result return type and unnecessary error handling in the create_bucket_from_metadata function. The function now directly returns a BucketOf<T> object.
  • pallets/funding/src/types.rs: Simplified the Bucket::new constructor by removing redundant parameters and calculations, and moved the 10% calculation logic into the constructor.

Updates to function calls:

  • Updated all calls to create_bucket_from_metadata across the module to remove .unwrap() since the function no longer returns a Result. This change affects:
    • pallets/funding/src/benchmarking.rs
    • pallets/funding/src/functions/1_application.rs
    • pallets/funding/src/instantiator/calculations.rs [1] [2] [3]
    • pallets/funding/src/tests/1_application.rs
    • pallets/funding/src/tests/3_auction.rs
    • pallets/funding/src/tests/misc.rs [1] [2]

Code cleanup:

Copy link
Member Author

lrazovic commented May 13, 2025

@lrazovic lrazovic requested a review from Copilot May 16, 2025 14:43
@lrazovic lrazovic changed the title chore: refactor buckets 🎨 Refactor the Price Buckets May 16, 2025
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 the bucket creation process and updates related documentation and tests. The changes remove unnecessary error handling by eliminating unwraps and adjust the bucket initialization to compute parameters internally.

  • Updated documentation for ProvideAssetPrice to remove the obsolete usd_decimals parameter.
  • Refactored bucket creation across modules to return BucketOf directly.
  • Modified tests and instantiator functions to align with the new bucket API.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
polimec-common/common/src/lib.rs Removed outdated usd_decimals doc comment to match updated API.
pallets/funding/src/types.rs Updated Bucket::new usage to compute delta_amount and delta_price.
pallets/funding/src/tests/*.rs Removed unwrap calls to adapt to the new bucket creation API.
pallets/funding/src/instantiator/calculations.rs Adjusted bucket creation to remove error handling.
pallets/funding/src/functions/misc.rs Changed create_bucket_from_metadata to return BucketOf directly.
Comments suppressed due to low confidence (2)

polimec-common/common/src/lib.rs:387

  • The documentation line for 'usd_decimals' has been removed to align with the updated function signature. Ensure all related documentation and code examples are updated accordingly.
/// * `usd_decimals`: The number of decimal places for the USD (or pricing) currency.

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

  • [nitpick] The function 'create_bucket_from_metadata' now returns BucketOf directly, removing error handling. Confirm that this change is safe because the fixed point conversion used internally can never fail, otherwise consider reintroducing error management.
pub fn create_bucket_from_metadata(metadata: &ProjectMetadataOf<T>) -> BucketOf<T> {

@lrazovic lrazovic requested review from Copilot and dastansam May 16, 2025 14:46
@lrazovic lrazovic self-assigned this May 16, 2025
@lrazovic lrazovic marked this pull request as ready for review May 16, 2025 14:46
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 the price bucket logic in the pallets/funding module by simplifying the bucket creation functions and cleaning up redundant error handling. Key changes include:

  • Eliminating the Result return type from create_bucket_from_metadata and removing unnecessary unwrap calls.
  • Simplifying the Bucket::new constructor to internally compute a 10% delta_price and delta_amount.
  • Minor documentation cleanup in polimec-common.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
polimec-common/common/src/lib.rs Removed an unused parameter from trait documentation.
pallets/funding/src/types.rs Updated Bucket::new constructor to encapsulate 10% calculation logic.
pallets/funding/src/functions/misc.rs Updated create_bucket_from_metadata to return BucketOf directly.
Various tests files in pallets/funding/src/tests/ and instantiator Removed unwrap calls in bucket creation to match the new API.
pallets/funding/src/functions/1_application.rs and benchmarking.rs Updated bucket creation call sites by removing .unwrap().

pub fn new(amount_left: Balance, initial_price: Price) -> Self {
let ten_percent = Percent::from_percent(10);
let delta_amount = ten_percent * amount_left;
let ten_percent_fixed = Price::checked_from_rational(1, 10).expect("10% is a valid fixed point number");
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.

Consider adding a comment explaining why the conversion using checked_from_rational with an expect call is guaranteed to succeed, to clarify that 10% is always representable.

Copilot uses AI. Check for mistakes.
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 9c7636a to 67b6aa1 Compare May 19, 2025 06:15
@lrazovic lrazovic force-pushed the 05-13-chore_refactor_buckets branch 2 times, most recently from 625c74f to a158df9 Compare May 19, 2025 06:48
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 67b6aa1 to a998aff Compare May 19, 2025 06:48
@lrazovic lrazovic force-pushed the 05-13-chore_refactor_buckets branch from a158df9 to d4d5667 Compare May 20, 2025 06:21
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from a998aff to 0d7c547 Compare May 20, 2025 06:21
@dastansam dastansam force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 0d7c547 to 992689e Compare May 23, 2025 11:43
@dastansam dastansam force-pushed the 05-13-chore_refactor_buckets branch from d4d5667 to 2cb9446 Compare May 23, 2025 11:43
@dastansam dastansam mentioned this pull request May 23, 2025
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 992689e to aa8e672 Compare May 28, 2025 13:35
@lrazovic lrazovic force-pushed the 05-13-chore_refactor_buckets branch 2 times, most recently from 2dc95a6 to 0b832b8 Compare May 28, 2025 13:38
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from aa8e672 to 7fcb836 Compare May 28, 2025 13:38
@dastansam dastansam force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 7fcb836 to 33fe2fd Compare July 9, 2025 06:38
@dastansam dastansam force-pushed the 05-13-chore_refactor_buckets branch from 0b832b8 to 32ac644 Compare July 9, 2025 06:38
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 33fe2fd to 4783aa9 Compare July 25, 2025 14:19
@lrazovic lrazovic force-pushed the 05-13-chore_refactor_buckets branch from 32ac644 to a6f07a5 Compare July 25, 2025 14:19
@dastansam dastansam force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 4783aa9 to 33fe2fd Compare July 25, 2025 21:21
@dastansam dastansam force-pushed the 05-13-chore_refactor_buckets branch from a6f07a5 to 32ac644 Compare July 25, 2025 21:21
@lrazovic lrazovic force-pushed the 05-12-feat_remove_account_and_id_from_stored_value branch from 33fe2fd to 6daa42f Compare August 4, 2025 07:59
@lrazovic lrazovic force-pushed the 05-13-chore_refactor_buckets branch from 32ac644 to 003774e Compare August 4, 2025 08:00
@lrazovic lrazovic closed this Aug 26, 2025
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