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.

Sign up for GitHub

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

Jump to bottom

Introduces TypeWithDefault<T, D: Get<T>> #4034

Merged
merged 11 commits into from
May 6, 2024
Merged

Introduces TypeWithDefault<T, D: Get<T>> #4034

merged 11 commits into from
May 6, 2024

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Apr 9, 2024 •
edited
Loading

Needed for: polkadot-fellows/runtimes#248

This PR introduces a new type TypeWithDefault<T, D: Get<T>> to be able to provide a custom default for any type. This can, then, be used to provide the nonce type that returns the current block number as the default, to avoid replay of immortal transactions.

@gupnik gupnik added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Apr 9, 2024
@bkchr
Copy link
Member

bkchr commented Apr 9, 2024

Because we like different implementations, I came up with another one :D #4044 (which should be actually the one that requires the least changes)

@gupnik
Copy link
Contributor Author

gupnik commented Apr 9, 2024

Because we like different implementations, I came up with another one :D #4044 (which should be actually the one that requires the least changes)

Yeah, yours is much simpler :))

This was referenced Apr 16, 2024
Closed
Closed
@gupnik gupnik marked this pull request as ready for review April 30, 2024 04:56
@gupnik gupnik requested a review from a team as a code owner April 30, 2024 04:56
@gupnik gupnik changed the title Introduces NonceWithDefault<D: Get<N>, N: Nonce> Introduces TypeWithDefault<T, D: Get<T>> Apr 30, 2024
franciscoaguirre
franciscoaguirre approved these changes May 1, 2024
View reviewed changes
substrate/frame/system/src/tests.rs Outdated Show resolved Hide resolved
kianenigma
kianenigma approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I am still a bit unsure about the technicality of all the trait impls that are added, but in general looks good as it solves a security issue.

ggwpez
ggwpez reviewed May 2, 2024
View reviewed changes
substrate/primitives/runtime/src/type_with_default.rs
Comment on lines +17 to +18

//! Provides a type that wraps another type and provides a default value.
Copy link
Member

Choose a reason for hiding this comment

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

DQ but it wraps a value, not a type, or?

ggwpez
ggwpez reviewed May 2, 2024
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Maybe ValueWithDefault<T, D: Get<T>>?

@gupnik
Copy link
Contributor Author

gupnik commented May 3, 2024

Maybe ValueWithDefault<T, D: Get<T>>?

It's actually a type. For example, the mock is wrapping u64.

@ggwpez
Copy link
Member

ggwpez commented May 3, 2024

Maybe ValueWithDefault<T, D: Get<T>>?

It's actually a type. For example, the mock is wrapping u64.

But it does also contain a value (T, PhantomData<D>)? Anyway, it seems to do the job.

@gupnik gupnik added this pull request to the merge queue May 6, 2024
Merged via the queue into master with commit 73c89d3 May 6, 2024
148 checks passed
@gupnik gupnik deleted the gupnik/custom-nonce branch May 6, 2024 04:31
dcolley added a commit to metaspan/polkadot-sdk that referenced this pull request May 6, 2024
@dcolley
Merge branch 'master' of https://github.com/metaspan/polkadot-sdk
5db2d43 
* 'master' of https://github.com/metaspan/polkadot-sdk: (65 commits)
  Introduces `TypeWithDefault<T, D: Get<T>>` (paritytech#4034)
  Publish `polkadot-sdk-frame`  crate (paritytech#4370)
  Add validate field to prdoc (paritytech#4368)
  State trie migration on asset-hub westend and collectives westend (paritytech#4185)
  Fix: dust unbonded for zero existential deposit (paritytech#4364)
  Bridge: added subcommand to relay single parachain header (paritytech#4365)
  Bridge: fix zombienet tests (paritytech#4367)
  [WIP][CI] Add more GHA jobs (paritytech#4270)
  Allow for 0 existential deposit in benchmarks for `pallet_staking`, `pallet_session`, and `pallet_balances` (paritytech#4346)
  Deprecate `NativeElseWasmExecutor` (paritytech#4329)
  More `xcm::v4` cleanup and `xcm_fee_payment_runtime_api::XcmPaymentApi` nits (paritytech#4355)
  sc-tracing: enable env-filter feature (paritytech#4357)
  deps: update jsonrpsee to v0.22.5 (paritytech#4330)
  Add PoV-reclaim enablement guide to polkadot-sdk-docs (paritytech#4244)
  cargo: Update experimental litep2p to latest version (paritytech#4344)
  Bridge: ignore client errors when calling recently added `*_free_headers_interval` methods (paritytech#4350)
  Make parachain template great again (and async backing ready) (paritytech#4295)
  [Backport] Version bumps and reorg prdocs from 1.11.0 (paritytech#4336)
  HRMP - set `DefaultChannelSizeAndCapacityWithSystem` with dynamic values according to the `ActiveConfig` (paritytech#4332)
  Statement Distribution Per Peer Rate Limit (paritytech#3444)
  ...
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
@codekitz @ggwpez @hitchhooker
Introduces TypeWithDefault<T, D: Get<T>> (paritytech#4034)
5155eed 
Needed for: polkadot-fellows/runtimes#248

This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able
to provide a custom default for any type. This can, then, be used to
provide the nonce type that returns the current block number as the
default, to avoid replay of immortal transactions.

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
@codekitz @ggwpez @TarekkMA
Introduces TypeWithDefault<T, D: Get<T>> (paritytech#4034)
5d8ff3a 
Needed for: polkadot-fellows/runtimes#248

This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able
to provide a custom default for any type. This can, then, be used to
provide the nonce type that returns the current block number as the
default, to avoid replay of immortal transactions.

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
This was referenced Aug 21, 2024
Closed
Closed
This was referenced Oct 4, 2024
Closed
Merged
kianenigma
kianenigma reviewed Nov 5, 2024
View reviewed changes
substrate/primitives/runtime/src/type_with_default.rs
/// A type that wraps another type and provides a default value.
///
/// Passes through arithmetical and many other operations to the inner value.
#[derive(Encode, Decode, TypeInfo, Debug, MaxEncodedLen)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Crucially, this type should have not derived TypeInfo. It should have had a manual TypeInfo implementation that would mimic the internal T.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkchr bkchr bkchr left review comments

@kianenigma kianenigma kianenigma approved these changes

@ggwpez ggwpez ggwpez approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

Assignees
No one assigned
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gupnik @bkchr @ggwpez @franciscoaguirre @kianenigma @codekitz