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

[NFTs] Update attributes with offchain signature #13390

Merged
merged 31 commits into from
Feb 22, 2023
Merged

[NFTs] Update attributes with offchain signature #13390

merged 31 commits into from
Feb 22, 2023

Conversation

jsidorenko
Copy link
Contributor

@jsidorenko jsidorenko commented Feb 15, 2023 •
edited
Loading

This PR allows to set/update NFT's attributes by providing an offchain signature.
Attribute's deposit in that case will be taken from the origin and not the signer.

This approach significantly improves the user experience, since in a standard flow the user first needs to give a permission to modify his attributes, then the external account would need to submit an attribute update extrinsic.

A possible use-case would be the evolvable nft tickets. By doing some offchain actions, the NFT's holder could get a pre-signed update to his attributes and submit it whether he sees the value.

Cumulus Companion: paritytech/cumulus#2189

jsidorenko and others added 26 commits January 12, 2023 15:15
@jsidorenko
Allow to mint with the pre-signed signatures
bdf9f7d
@jsidorenko
Another try
320582d
@jsidorenko
WIP: test encoder
a92dc15
@jsidorenko
Fix the deposits
124d872
@jsidorenko
Refactoring + tests + benchmarks
bb91166
@jsidorenko
Add sp-core/runtime-benchmarks
9f7e563
@jsidorenko
Remove sp-core from dev deps
a4c7e79
@jsidorenko
Enable full_crypto for benchmarks
09f86aa
@jsidorenko
Typo
dc9ff18
@jsidorenko
Fix
55eeb12
@jsidorenko @gilescope
Update frame/nfts/src/mock.rs
e8ea9ec 
Co-authored-by: Squirrel <gilescope@gmail.com>
Merge branch 'master' of https://github.com/paritytech/substrate into…
5f27ced 
… HEAD
".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts
62edf5f
@jsidorenko
Add docs
b119156
@jsidorenko
Add attributes into the pre-signed object & track the deposit owner f…
781f834 
…or attributes
@jsidorenko
Update docs
839c37c
@jsidorenko
Merge branch 'master' into js/offchain-mint
4980270 
# Conflicts:
#	frame/nfts/src/weights.rs
Merge branch 'master' of https://github.com/paritytech/substrate into…
c2d7c99 
… HEAD
".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts
7120dd0
@jsidorenko
Add the number of attributes provided to weights
088dddd
@jsidorenko
Merge branch 'master' into js/offchain-mint
ad7b0e1
@jsidorenko
Support pre-signed attributes
603a194
@jsidorenko
Update docs
30b26a7
@jsidorenko
Merge branch 'master' into js/offchain-attributes
22c176d 
# Conflicts:
#	frame/nfts/src/benchmarking.rs
#	frame/nfts/src/common_functions.rs
#	frame/nfts/src/lib.rs
#	frame/nfts/src/mock.rs
#	frame/nfts/src/tests.rs
#	frame/nfts/src/types.rs
#	frame/nfts/src/weights.rs
@jsidorenko
Fix merge artifacts
2c43623
@jsidorenko
Update docs
1a9fdee
@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 Feb 15, 2023
@ruseinov
Copy link
Contributor

This is mostly looking good to me, just asking some questions, since I'm not very familiar with the codebase of this.

@command-bot
Copy link

command-bot bot commented Feb 15, 2023

@jsidorenko Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2394914 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2394914/artifacts/download.

@jsidorenko
Copy link
Contributor Author

bot bench $ pallet dev pallet_nfts

@command-bot
Copy link

command-bot bot commented Feb 15, 2023 •
edited
Loading

@jsidorenko https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2395409 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 24-e99906ad-b4f4-49d8-9ba0-8df0084689e7 to cancel this command or bot cancel to cancel all commands in this pull request.

@ruseinov
Copy link
Contributor

One other thing that I have noticed - this branch seems to be based on a branch that has already been merged to master, so the commit history is kinda messy. I know this does not matter much with squash and merge, just pointing it out as it makes things a bit confusing, though the diff is pretty clear.

@ruseinov
Copy link
Contributor

Also, if there is a tracking issue for that - it might be great to mention it in the PR description. And potentially make sure that the PR closes it.

@ruseinov
Copy link
Contributor

One other thing that I have noticed - this branch seems to be based on a branch that has already been merged to master, so the commit history is kinda messy. I know this does not matter much with squash and merge, just pointing it out as it makes things a bit confusing, though the diff is pretty clear.

This is happening, because the same branch is being used that has already been merged. For future work I would definitely make sure that the changeset is based on master to avoid confusion for both reviewers and posterity.

@ruseinov ruseinov self-requested a review February 15, 2023 11:18
ruseinov
ruseinov approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@ruseinov ruseinov left a comment

Choose a reason for hiding this comment

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

The changeset looks good to me, apart from the issues I mentioned that have to do with commit history.

@jsidorenko
Copy link
Contributor Author

I agree, I needed to cherry-pick the commits. I tried to do the rebase and got into the conflicts hell, so decided just to use merge :)

@jsidorenko jsidorenko mentioned this pull request Feb 15, 2023
Closed
15 tasks
@command-bot
Copy link

command-bot bot commented Feb 15, 2023

@jsidorenko Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_nfts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2395409 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2395409/artifacts/download.

@jsidorenko jsidorenko mentioned this pull request Feb 15, 2023
Merged
joepetrowski
joepetrowski approved these changes Feb 22, 2023
View reviewed changes
frame/nfts/src/types.rs Outdated Show resolved Hide resolved
jsidorenko and others added 2 commits February 22, 2023 15:11
@jsidorenko @joepetrowski
Update frame/nfts/src/types.rs
5e81316 
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@jsidorenko
Update types.rs
72bcea2
@jsidorenko
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b0ed48e into master Feb 22, 2023
@paritytech-processbot paritytech-processbot bot deleted the js/offchain-attributes branch February 22, 2023 13:50
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@jsidorenko @gilescope
@joepetrowski @ark0f
[NFTs] Update attributes with offchain signature (paritytech#13390)
7ad96a7 
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Ank4n pushed a commit that referenced this pull request Feb 28, 2023
@jsidorenko @gilescope
@joepetrowski @Ank4n
[NFTs] Update attributes with offchain signature (#13390)
382484f 
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@mmostafas mmostafas added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 6, 2023
@kiltbot kiltbot mentioned this pull request Apr 3, 2023
Closed
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
@jsidorenko @gilescope
@joepetrowski @ukint-vs
[NFTs] Update attributes with offchain signature (paritytech#13390)
2104709 
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@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 @gilescope
@joepetrowski @nathanwhit
[NFTs] Update attributes with offchain signature (paritytech#13390)
8f6310d 
* Allow to mint with the pre-signed signatures

* Another try

* WIP: test encoder

* Fix the deposits

* Refactoring + tests + benchmarks

* Add sp-core/runtime-benchmarks

* Remove sp-core from dev deps

* Enable full_crypto for benchmarks

* Typo

* Fix

* Update frame/nfts/src/mock.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add docs

* Add attributes into the pre-signed object & track the deposit owner for attributes

* Update docs

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Add the number of attributes provided to weights

* Support pre-signed attributes

* Update docs

* Fix merge artifacts

* Update docs

* Add more tests

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nfts

* Update frame/nfts/src/types.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update types.rs

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ruseinov ruseinov ruseinov approved these changes

@joepetrowski joepetrowski joepetrowski 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. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsidorenko @ruseinov @joepetrowski @mmostafas