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

Change identity raw data values to be generic Get<u32> #3977

Open
2 tasks done
ltfschoen opened this issue Apr 4, 2024 · 10 comments
Open
2 tasks done

Change identity raw data values to be generic Get<u32> #3977

ltfschoen opened this issue Apr 4, 2024 · 10 comments
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I5-enhancement An additional feature request.

Comments

@ltfschoen
Copy link

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

Motivation mentioned here: https://github.com/polkadot-fellows/RFCs/pull/76/files#diff-26a34bd4a30949abd8e394e45855154a61baf508856915c9ebc105726c28e502R13

Request

Change identity raw data values to be generic Get to allow larger lengths instead of restricted to 32 bytes/chars limit.

Solution

Solution as established here polkadot-fellows/RFCs#76

This Data type is currently re-used across both the new Rococo config and partly in Polkadot.
It should be enough to make that 32 there generic as Get for the identity.
Then in a second change you can increase it for the Polkadot runtime config.
So first a PR to polkadot-sdk to make that value configurable and then we can propose the change in the pallet config.

Are you willing to help with this request?

Yes!

@ltfschoen ltfschoen added the I5-enhancement An additional feature request. label Apr 4, 2024
@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label Apr 4, 2024
@bkchr
Copy link
Member

bkchr commented Apr 4, 2024

Please go ahead with a pr @ltfschoen

@ggwpez ggwpez removed the I10-unconfirmed Issue might be valid, but it's not yet known. label Apr 4, 2024
@ltfschoen
Copy link
Author

I can't work on this right at the moment, I'm trying to apply for the PBA "Remote Track" cohort to improve my knowledge

@ggwpez
Copy link
Member

ggwpez commented Apr 6, 2024

Okay thanks for letting us know. Will mark as mentor.

@ggwpez ggwpez added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. labels Apr 6, 2024
@ltfschoen
Copy link
Author

ltfschoen commented Apr 7, 2024

so i've run a local node using the Polkadot SDK and exposed RPC port 9944, then if i use the current Polkadot.js Apps UI that uses Polkadot.js api v10.12.4 and do the following, I recreate the error:

createType(Call):: Call: failed decoding identity.setIdentity:: Struct: failed on args: {"info":"{\"additional\":\"Vec<(Data,Data)>\",\"display\":\"Data\",\"legal\":\"Data\",\"web\":\"Data\",\"riot\":\"Data\",\"email\":\"Data\",\"pgpFingerprint\":\"Option<[u8;20]>\",\"image\":\"Data\",\"twitter\":\"Data\"}"}:: Struct: failed on info: {"additional":"Vec<(Data,Data)>","display":"Data","legal":"Data","web":"Data","riot":"Data","email":"Data","pgpFingerprint":"Option<[u8;20]>","image":"Data","twitter":"Data"}:: Struct: failed on email: {"_enum":{"None":"Null","Raw":"Bytes","BlakeTwo256":"H256","Sha256":"H256","Keccak256":"H256","ShaThree256":"H256"}}:: Data.Raw values are limited to a maximum length of 32 bytes

So I then tried forking that branch of https://github.com/polkadot-js/api

git clone git@github.com:ltfschoen/api.git
cd api
git remote add upstream https://github.com/polkadot-js/api
git fetch upstream v10.12.4:v10.12.4
git checkout v10.12.4

Searched for the snippet of that error text Call: failed decoding throughout that codebase to find that it is triggered here https://github.com/polkadot-js/api/blob/v10.12.4/packages/types/src/generic/Call.ts#L144

But to check that my Get<u32> related identity changes to the Substrate Node will work in Polkadot.js Apps by modifying that version of the Polkadot.js API, it's necessary to first run my own local version of Poladot.js Apps, and customise the Polkadot.js API dependency that it uses so that it uses the latest metadata from my Substrate Node by following the steps described here https://github.com/polkadot-js/api/blob/v10.12.4/packages/types/src/metadata/README.md, which would be quite time consuming.

So I instead tried using SubXT https://github.com/paritytech/subxt to recreate creating that identity > setIdentity extrinsic, but I encountered errors that I didn't understand how to overcome and added TODOs in the code to highlight what i didn't understand. Here's the PR that shows the changes that I made and the error is shown in the subject of the PR ltfschoen/subxt#1

@ltfschoen
Copy link
Author

i seem to have fixed that by defining

impl<Data> From<Vec<Data>> for BoundedVec4<Data> {
	fn from(v: Vec<Data>) -> Self {
		BoundedVec4(v)
	}
}

but i don't know why i'm using BoundedVec4 instead of BoundedVec

@ltfschoen
Copy link
Author

but i was getting a timeout when connecting to Substrate node, so i tried implementing reconnecting_jsonrpsee_ws_client, but that seems to have required me to use the newtype pattern, and now i'm getting an error ltfschoen/subxt#1 (comment)

@ggwpez
Copy link
Member

ggwpez commented Apr 7, 2024

I dont get what is going on here. What are your trying to do with PJS and subxt?
This should be a runtime-only change.

@ltfschoen
Copy link
Author

ltfschoen commented Apr 7, 2024

I guess I should just start by trying to write a unit test within Polkadot SDK that replicates the RPC call error Call: failed decoding identity.setIdentity due to taking the input of an email address that's too long to setIdentity(info) where the email Data::Raw value is say abcdefghijklmnopqrstuvwxyz@me.com, and then try to change the Polkadot SDK identity runtime implementation until that test passes when running cargo test -p pallet-identity.

I was effectively trying to write the equivalent of an e2e test instead with an external tool like PJS or subxt to create a JSON-RPC call to a locally running Polkadot SDK Substrate node with development environment debugging logs activated so I could debug and know whether my changes to the Polkadot SDK identity runtime implementation actually work and solve the problem.

@ggwpez
Copy link
Member

ggwpez commented Apr 8, 2024

Okay i see. You can first do the changes with a unit test of setIdentity (which should be enough testing in itself), and then if you still want to try it manually once just run with local node and PJS.
We dont have integration testing with PJS, subxt or RPC stuff for normal runtime changes.

@ltfschoen
Copy link
Author

Ok thanks, I'm working on it in this PR https://github.com/paritytech/polkadot-sdk/pull/4043/files. I'm not sure where to start with the implementation though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. I5-enhancement An additional feature request.
Projects
Status: Backlog
Development

No branches or pull requests

3 participants