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

Fix wrong allowed_currencies_len variable #298

Conversation

ebma
Copy link
Member

@ebma ebma commented Aug 15, 2023

Changes the allowed_currencies_len to use the length of the updated AllowedCurrencies for the second size check (instead of doing a redundant check on the passed currencies vector).

Also adds a similar check to the remove_allowed_currencies extrinsic. While in theory, it would of course never be possible to remove more currencies than are contained in the AllowedCurrencies storage item, which is limited to the MaxAllowedCurrencies config parameter in our add_allowed_currencies function, the caller could still provide a very large and valid currencies vector to this function and thus bloat the chain. That's why also checking the max size of the vector makes sense here.

Closes #297.

@ebma ebma requested a review from a team August 15, 2023 14:26
Copy link
Contributor

@adelarja adelarja left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

Why are the weight definitions of these two extrinsics add_allowed_currencies(T::MaxAllowedCurrencies::get()) and remove_allowed_currencies(T::MaxAllowedCurrencies::get()) instead of taking the actual length of the currencies argument into account, i.e.:

  • add_allowed_currencies(currencies.len() as u32) and
  • remove_allowed_currencies(currencies.len() as u32)

This is what I originally referred to in this comment.

I think that would eliminate all problems.

@ebma
Copy link
Member Author

ebma commented Aug 15, 2023

@TorstenStueber as you can see in the code that you link to in your other comment, when defining the weight in the [pallet::weight] macro, they also use the config variable that represents the maximum possible value. If you then look at the actual definition of the set_sub_olds weight function declaration here, it's declared using a range variable. This is exactly how we also do it, defining the maximum possible value here but using a range variable in the weight function here.

Copy link
Member

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

This difference is of course that they also add T::WeightInfo::set_subs_new(subs.len() as u32). But that's really a nitpick. Your solution is fine.

@ebma ebma merged commit 05607a1 into main Aug 15, 2023
@ebma ebma deleted the 297-amend-pdm-007-fix-incorrect-allowed-currency-counting-in-currencies-allowance-pallet branch August 15, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Amend PDM 007): Fix incorrect allowed-currency counting in currencies allowance pallet
3 participants