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

Easy flow to set an identity #404

Merged
merged 10 commits into from
Oct 18, 2023
Merged

Easy flow to set an identity #404

merged 10 commits into from
Oct 18, 2023

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Oct 16, 2023

closes #373

Adds an easy setup to set or modify and on-chain identity

image
image


Submission checklist:

Layout

  • Change inspected in the desktop web ui
  • Change inspected in the mobile web ui

Compatibility

  • Functionality of change validated with a connected account with multisig
  • Applicable elements hidden / disabled for watched multisigs / pure
  • Looks good for solo multisig
  • Looks good for multisig with proxy

@asnaith
Copy link
Member

asnaith commented Oct 16, 2023

This is a really nice improvement, working great for adding and updating, all working as expected.

I found a crash in the case where an address is pasted into one of the inputs (see the video where I just copied from the identicon and then pasted). Pasting in normal strings works fine.

CleanShot.2023-10-16.at.15.36.18.mp4

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 16, 2023

Ah great catch, I didn't realize there was a limit of char. I'll implement this.

Copy link
Collaborator

@Lykhoyda Lykhoyda left a comment

Choose a reason for hiding this comment

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

The UI works as expected, I was able to create a transaction and sign and my multisig name was changed.

TBH for me as a quite new person to the Polkadot ecosystem the field data labels are confusing, it's hard to guess where all this data would be reflected. Can we at least add input placeholders to give more information about the data we ask?

Comment on lines 25 to 30
export const easyTransferTitle = [
'Send tokens',
'From call data',
'Manual extrinsic',
'Set identity'
] as const
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably it's a case to enum:
enum EasyTransferTitle {
SendTokens = 'Send tokens',
FromCallData = 'From call data',
ManualExtrinsic = 'Manual extrinsic',
SetIdentity = 'Set identity',
}

Copy link
Collaborator Author

@Tbaut Tbaut Oct 17, 2023

Choose a reason for hiding this comment

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

why? I believe you misinterpret the strict typing that is used here. This is used as a type (right next line), and allows to strongly type anything that uses EasyTransferTitle while remaining very easy to read. Also it allows to do things like easyTransferTitle.included(val) to check whether this value passed is actually of the type we expect, and not just a random string.

return {
[SEND_TOKEN_MENU]: (
'Send tokens': (
Copy link
Collaborator

Choose a reason for hiding this comment

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

With enum, it will be easier to use the value without specifying a string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is no a string, this is a EasyTransferTitle. You can't put any string in there without a type error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I saw the type, it's still confusing to read. I could understand if you created a type with one-word strings: like active | disabled | focus and used them this way. But for a 2-3 word string, it's probably the case for an enum where you don't use directly the value but the link to it. You can use the whole sentence as a type, but it doesn't mean you should do it. It works both ways, but in my opinion with an enum, it gives better readability and expectation that this is a predefined value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, not willing to fight too much for this one, I agree that enum feels safer (although it's actually the same). Did the change.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 17, 2023

Can we at least add input placeholders to give more information about the data we ask?

Sure that's a good idea. Just to be clear, Multix, and multisigs in general are not for ppl new to Polkadot, but it doesn't mean that I can use this as an argument for everything! The goal of this component is to make it so that ppl don't have to use polkadot.js/apps, they had no other option this far. Pjs has an easy flow for setting an identity that looks similar.

image

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 17, 2023

I hope you had the chance to test it out, because on Rococo it's now filtered 😭 because of paritytech/polkadot-sdk#1476
It's still valid for all the other chains though

@asnaith
Copy link
Member

asnaith commented Oct 17, 2023

The recent updates, character limits, placeholder text, and title updates, are nice additions.

The "Display name" error and highlighted field appear immediately upon loading the modal, which effectively signals that it's a mandatory field. This seems fine to me.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 18, 2023

The "Display name" error and highlighted field appear immediately upon loading the modal, which effectively signals that it's a mandatory field. This seems fine to me.

Yeah that's not good, I'll change this before merging.

@Tbaut Tbaut merged commit 4142873 into main Oct 18, 2023
7 checks passed
@Tbaut Tbaut deleted the tbaut-set-identity branch October 18, 2023 15:39
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.

Add an easy tx flow to set an identity
3 participants