Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add FixedPointOperand blanket implementation #14634

Merged
merged 8 commits into from
Aug 2, 2023
Merged

Add FixedPointOperand blanket implementation #14634

merged 8 commits into from
Aug 2, 2023

Conversation

juangirini
Copy link
Contributor

@juangirini juangirini commented Jul 25, 2023 •
edited
Loading

This PR replaces the explicit FixedPointOperand impls by a blanket one so that Balance implements it.

@juangirini juangirini requested review from a team July 25, 2023 14:22
@juangirini juangirini self-assigned this Jul 25, 2023
@juangirini juangirini added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. T1-runtime This PR/Issue is related to the topic “runtime”. labels Jul 25, 2023
@juangirini
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Jul 25, 2023 •
edited
Loading

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3257301 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 17-c4e0b7b9-4636-4fed-ba46-90177f085715 to cancel this command or bot cancel to cancel all commands in this pull request.

@juangirini juangirini added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Jul 25, 2023
@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3257301 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3257301/artifacts/download.

gavofyork
gavofyork reviewed Jul 25, 2023
View reviewed changes
frame/assets/src/lib.rs Outdated Show resolved Hide resolved
gavofyork
gavofyork suggested changes Jul 25, 2023
View reviewed changes
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Explicitly stating the bound should not be needed.

@juangirini juangirini changed the title Bound Balance to FixedPointOperand Update Balance bound and FixedPointOperand implementation Jul 26, 2023
@juangirini juangirini changed the title Update Balance bound and FixedPointOperand implementation Refactor Balance bounds and FixedPointOperand implementation Jul 26, 2023
@juangirini
Copy link
Contributor Author

I haven't found that blanket impl so I added it as part of this PR. Also, Copy + AtLeast32BitUnsigned are missing CheckedNeg compared to FixedPointOperand so I added that too.

@juangirini juangirini requested review from gavofyork, a team, bkchr, pgherveou and athei July 26, 2023 11:20
@juangirini juangirini mentioned this pull request Jul 26, 2023
Merged
athei
athei reviewed Jul 26, 2023
View reviewed changes
frame/nis/src/lib.rs Outdated Show resolved Hide resolved
@juangirini juangirini requested a review from a team July 31, 2023 10:13
gavofyork
gavofyork reviewed Jul 31, 2023
View reviewed changes
frame/assets/src/lib.rs Outdated
@@ -241,7 +243,8 @@ pub mod pallet {
+ Copy
+ MaybeSerializeDeserialize
+ MaxEncodedLen
+ TypeInfo;
+ TypeInfo
+ CheckedNeg;
Copy link
Member

@gavofyork gavofyork Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Avoid adding here - just add it into BaseArithmetic trait/blanket impl along with all the other Checked* bounds. With that in place you should be able to remove all the other CheckedNeg stuff in this PR and simplify a bit.

gavofyork
gavofyork suggested changes Jul 31, 2023
View reviewed changes
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Getting there. Left a comment.

@juangirini juangirini changed the title Refactor Balance bounds and FixedPointOperand implementation Update BaseArithmetic bounds and FixedPointOperand implementation Jul 31, 2023
@juangirini juangirini changed the title Update BaseArithmetic bounds and FixedPointOperand implementation Add FixedPointOperand blanket implementation Jul 31, 2023
@juangirini juangirini requested a review from gavofyork August 1, 2023 08:49
gavofyork
gavofyork approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

LGTM.

@juangirini
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 4d9d911 into master Aug 2, 2023
@paritytech-processbot paritytech-processbot bot deleted the jg/add-fixed-point-operand-to-balance branch August 2, 2023 07:54
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@athei athei athei approved these changes

@pgherveou pgherveou pgherveou approved these changes

@gavofyork gavofyork gavofyork approved these changes

@bkchr bkchr Awaiting requested review from bkchr

Assignees

@juangirini juangirini

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juangirini @gavofyork @pgherveou @athei