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

Add MaxEncodeLen to implement_per_thing! #10715

Merged
merged 7 commits into from
Jan 21, 2022
Merged

Add MaxEncodeLen to implement_per_thing! #10715

merged 7 commits into from
Jan 21, 2022

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 21, 2022

  • Add MaxEncodeLen to implement_per_thing!
  • Test that Percent et.al. can be used in Storage. 👉 I'm open to have a better way to test this.

Closes #10712

@ggwpez
fix doc
d88cc27 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Add MaxEncodedLen to implement_per_thing!
5c99b2c 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Test that Percent et.al. can be used in Storage.
52558af 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy 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 labels Jan 21, 2022
@ggwpez ggwpez self-assigned this Jan 21, 2022
@ggwpez
Add comment
27a96f6 
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez marked this pull request as ready for review January 21, 2022 14:40
@ggwpez ggwpez removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 21, 2022
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 21, 2022
kianenigma
kianenigma reviewed Jan 21, 2022
View reviewed changes
frame/support/test/tests/pallet.rs Outdated
@@ -535,6 +535,27 @@ pub mod pallet4 {
impl<T: Config> Pallet<T> {}
}

/// Test that `Percent`, `PerU16`, `Permill`, `Perbill` and `Perquintill`
Copy link
Contributor

@kianenigma kianenigma Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I honestly think this is very out of context here.

Like, why should frame-support test that an external library type implements some trait? Should we add tests here to make sure all sp-blah-blah types implement our desired traits, from the PoV of frame? clearly no.

I totally agree though that we should test for this, but this test should live in sp-arithmetic, not here. There are similar tests there and that ensure all P: PerThings implement Compact for example. This is exactly an identical scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the pointer to Compact! The test just became trivial 😆

kianenigma
kianenigma reviewed Jan 21, 2022
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.

Strongly feel that the test should be in sp-arithmetic, otherwise all good.

kianenigma
kianenigma suggested changes Jan 21, 2022
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.

trying to prevent merge until #10715 (comment) is resolved.

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 227a94f into master Jan 21, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-percent-maxencode branch January 21, 2022 17:08
eskimor pushed a commit that referenced this pull request Jan 27, 2022
@ggwpez @eskimor
Add MaxEncodeLen to implement_per_thing! (#10715)
33b0f37 
* fix doc

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add MaxEncodedLen to implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test that Percent et.al. can be used in Storage.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add comment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Test that Percent et.al. can be used in Storage."

This reverts commit 52558af.

* Test MaxEncodedLen in implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Add comment"

This reverts commit 27a96f6.
@ggwpez ggwpez mentioned this pull request Feb 1, 2022
Closed
Wizdave97 pushed a commit to ComposableFi/substrate that referenced this pull request Feb 4, 2022
@ggwpez @Wizdave97
Add MaxEncodeLen to implement_per_thing! (paritytech#10715)
ad82600 
* fix doc

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add MaxEncodedLen to implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test that Percent et.al. can be used in Storage.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add comment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Test that Percent et.al. can be used in Storage."

This reverts commit 52558af.

* Test MaxEncodedLen in implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Add comment"

This reverts commit 27a96f6.
This was referenced Mar 4, 2022
Closed
Closed
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@ggwpez @grishasobol
Add MaxEncodeLen to implement_per_thing! (paritytech#10715)
cd99081 
* fix doc

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add MaxEncodedLen to implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test that Percent et.al. can be used in Storage.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add comment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Test that Percent et.al. can be used in Storage."

This reverts commit 52558af.

* Test MaxEncodedLen in implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Add comment"

This reverts commit 27a96f6.
@Dengjianping Dengjianping mentioned this pull request Apr 19, 2022
Merged
10 tasks
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@ggwpez @ark0f
Add MaxEncodeLen to implement_per_thing! (paritytech#10715)
6e9cc80 
* fix doc

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add MaxEncodedLen to implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test that Percent et.al. can be used in Storage.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add comment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Test that Percent et.al. can be used in Storage."

This reverts commit 52558af.

* Test MaxEncodedLen in implement_per_thing!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Add comment"

This reverts commit 27a96f6.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@kianenigma kianenigma kianenigma approved these changes

@bkchr bkchr bkchr approved these changes

Assignees

@ggwpez ggwpez

Labels
A0-please_review Pull request needs code review. 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trait PerThing missing MaxEncodedLen
4 participants
@ggwpez @kianenigma @shawntabrizi @bkchr