Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support more types in TypeWithDefault #6411

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

NingLin-P
Copy link
Contributor

@NingLin-P NingLin-P commented Nov 8, 2024

Description

When using TypeWithDefault<u32, ..> as the default nonce provider to overcome the replay attack issue, it fails to compile due to TypeWithDefault<u32, ..>: TryFrom<u64> is not satisfied (which is required by trait BaseArithmetic).

This is because the blanket implementation TryFrom<U> for T where U: Into<T> only impl TryFrom<u16> and TryFrom<u8> for u32 since u32 only impl Into for u16 and u8 but not u64.

This PR fixes the issue by adding TryFrom<u16/u32/u64/u128> and From<u8/u16/u32/u64/u128> impl (using macro) for TypeWithDefault<u8/u16/u32/u64/u128, ..> and removing the blanket impl (otherwise the compiler will complain about conflicting impl), such that TypeWithDefault<u8/u16/u32/u64/u128, ..>: AtLeast8/16/32Bit is satisfied.

Integration

This PR adds support to more types to be used with TypeWithDefault, existing code that used u64 with TypeWithDefault should not be affected, an unit test is added to ensure that.

Review Notes

This PR simply makes TypeWithDefault<u8/u16/u32/u64/u128, ..>: AtLeast8/16/32Bit satisfied

@bkchr bkchr added the T17-primitives Changes to primitives that are not covered by any other label. label Nov 8, 2024
@github-actions github-actions bot requested review from bkchr and gupnik November 8, 2024 11:56
Copy link

github-actions bot commented Nov 8, 2024

Review required! Latest push from author must always be reviewed

prdoc/pr_6411.prdoc Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Nov 10, 2024

@NingLin-P there are still build issues. Could you please fix them?

Signed-off-by: linning <[email protected]>
@NingLin-P
Copy link
Contributor Author

Sorry about the force push noise! the previous solution does not work because Nonce requires AtLeast32Bit: BaseArithmetic + From<u16> + From<u32> and the From<u16/u32> impl is removed due to conflicting trait impl. The new commit used another way to fix the issue, instead of using blanket impl it uses macro to add TryFrom and From impl for every TypeWithDefault<u8/u16/u32/u64/u128, ..> to ensure AtLeast8/16/32Bit is satisfied.

@NingLin-P
Copy link
Contributor Author

@bkchr could you take a look, please?

Signed-off-by: linning <[email protected]>
@bkchr bkchr enabled auto-merge November 14, 2024 20:32
@bkchr bkchr added this pull request to the merge queue Nov 14, 2024
Merged via the queue into paritytech:master with commit 5bc571b Nov 14, 2024
194 of 198 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T17-primitives Changes to primitives that are not covered by any other label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants