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

[NFTs] Improve offchain signature validation #13960

Merged
merged 2 commits into from
Apr 20, 2023
Merged

[NFTs] Improve offchain signature validation #13960

merged 2 commits into from
Apr 20, 2023

Conversation

jsidorenko
Copy link
Contributor

When we try to sign some data using the UIs like PolkadotJS apps, PolkadotJS extension + some others, all of them implicitly wrap the data value into the tag. That behaviour leads to different signatures generated, which look incorrect from the pallet's side.
This PR improves the offchain signature's validation by adding the support of wrapped into data values

@jsidorenko jsidorenko 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 20, 2023
@jsidorenko jsidorenko requested a review from ggwpez April 20, 2023 10:51
ggwpez
ggwpez approved these changes Apr 20, 2023
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.

Looks good.

Did you try it with PolkadotJS that it works? Just to make sure that there are no differences in the encoding.

frame/nfts/src/common_functions.rs Outdated Show resolved Hide resolved
@jsidorenko
Copy link
Contributor Author

Looks good.

Did you try it with PolkadotJS that it works? Just to make sure that there are no differences in the encoding.

Yeah, was trying it many times until I got that working :)

gilescope
gilescope approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@gilescope gilescope 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 with the early exit if signature passes.

@jsidorenko
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit d8c3209 into master Apr 20, 2023
@paritytech-processbot paritytech-processbot bot deleted the js/improve-signature-validation branch April 20, 2023 12:38
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@jsidorenko @nathanwhit
[NFTs] Improve offchain signature validation (paritytech#13960)
67c7096 
* Improve signature validation

* Rework
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@gilescope gilescope gilescope approved these changes

Assignees
No one assigned
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jsidorenko @gilescope @ggwpez