paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Remove unrequired check #14298

Closed gilescope closed 1 year ago

gilescope commented 1 year ago

The get_pool_account function no longer relies on a _truncating function when creating the account (which could have lead to pool collisions), and there is a test in place should someone alter the function in the future to something that created pool collisions.

I don't think this check is helpful any longer and it's now inaccurate as there's now no reason for the account type to be at least u128.

Fixes issues brought to attention in #14289.

gilescope commented 1 year ago

The check is also on lib.rs when it imports the module. It’s nicer if the check is at the lib.rs level as rustc won’t load that file from disk and also any changes to that file won’t trigger incremental compilation dirtying (unless the feature is activated)

On Sun, 4 Jun 2023 at 11:37, Liam Aharon @.***> wrote:

@.**** commented on this pull request.

In frame/asset-conversion/src/benchmarking.rs https://github.com/paritytech/substrate/pull/14298#discussion_r1216530004 :

-#![cfg(feature = "runtime-benchmarks")]

Why's this removed? I notice it exists in most other benchmarking.rs files.

— Reply to this email directly, view it on GitHub https://github.com/paritytech/substrate/pull/14298#pullrequestreview-1461102710, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGEJCAJ6DYUC3JVYDPADQTXJRQNTANCNFSM6AAAAAAYZGZVAQ . You are receiving this because you authored the thread.Message ID: @.***>

gilescope commented 1 year ago

bot merge

paritytech-processbot[bot] commented 1 year ago

Waiting for commit status.

paritytech-processbot[bot] commented 1 year ago

Merge cancelled due to error. Error: Statuses failed for 43adad4c1b8ce4b54f5b44441fc74e7b38bf3642

gilescope commented 1 year ago

bot merge