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

identity directory milestone 1 delivery #229

Merged
merged 4 commits into from
Oct 18, 2021

Conversation

DeFiYaco
Copy link
Contributor

Milestone Delivery Checklist

Link to the application PR: w3f/Grants-Program#255

@DeFiYaco DeFiYaco force-pushed the identity-directory-milestone1 branch from d68b42f to ead113f Compare July 15, 2021 08:29
@DeFiYaco DeFiYaco force-pushed the identity-directory-milestone1 branch from ead113f to f9b79e7 Compare July 15, 2021 09:01
@mmagician
Copy link
Contributor

Thanks @Jakic007 for the delivery of the two milestones! We'll look into it as soon as we can.

@alxs alxs self-assigned this Jul 15, 2021
@alxs
Copy link
Contributor

alxs commented Jul 19, 2021

Hi @Jakic007, thanks for the delivery. I have a few initial remarks:

  • What about the name? See substrate identity directory Grants-Program#255 (comment).
  • It was also mentioned in the application that the service would be hosted on its own domain.
  • Is the version on GitHub the same as live on https://identity.shardlabs.io/? That version still doesn't load for me most of the time and has a bunch of not-so-nice UX issues.
  • Could you be a bit more precise with the links for the deliverables? Currently they all point to the repository, it would be nice if you could point to where exactly each feature is implemented.

@alxs
Copy link
Contributor

alxs commented Jul 29, 2021

@Jakic007 any updates here?

@DeFiYaco
Copy link
Contributor Author

Hi @Jakic007, thanks for the delivery. I have a few initial remarks:

* What about the name? See [substrate identity directory Grants-Program#255 (comment)](https://github.com/w3f/Grants-Program/pull/255#issuecomment-790643085).

* It was also mentioned in the application that the service would be hosted on its own domain.

* Is the version on GitHub the same as live on https://identity.shardlabs.io/? That version still doesn't load for me most of the time and has a bunch of not-so-nice UX issues.

* Could you be a bit more precise with the links for the deliverables? Currently they all point to the repository, it would be nice if you could point to where exactly each feature is implemented.
  • We have named this project Identity Hub
  • Web app is hosted on identityhub.xyz domain
  • There were some issues caused by polkadot node updates that would break this app if polkadotjs api dependency is not updated as well. That is now fixed and we are now running our nodes for Polkadot and Kusama chains so we can update nodes and dependencies at the same time without breaking the app functionalities
  • Deliverables are updated for milestone 1 & 2

@alxs
Copy link
Contributor

alxs commented Jul 30, 2021

Thanks. A few more remarks:

I would recommend you to go through an extensive review of the project as it currently seems to be broken anywhere I look. I'd like to ask you to please do so and bring this to an acceptable state within 14 days as specified in the terms and conditions, seeing as the delivery is already delayed by over 3 months, or the grant may be terminated.

@MilGard91
Copy link
Contributor

Hi @alxs,
We changed the calculation for the balance but it still isn't the same as on polkascan, but we get the same number as in polkadot subscan, not sure how you want to handle this. This is the example of different balances on polkascan and polkadot subscan.

@alxs
Copy link
Contributor

alxs commented Aug 2, 2021

The balance displayed on your project is currently different to both that on Polkascan and Subscan for the account you just linked.

@MilGard91
Copy link
Contributor

I wanted to check with you before deploying it to the server.

@alxs
Copy link
Contributor

alxs commented Aug 2, 2021

See also PolkaStats which contains more info, not sure where the discrepancy comes from or which one is right.

@DeFiYaco
Copy link
Contributor Author

DeFiYaco commented Aug 3, 2021

* Governance and treasury don't work, which are part of the mockups

Governance and treasury are not expected to be delivered in Milestone 1 & 2. That was planned for possible extension as part of identity hub plugin system.
We can either collapse these 2 cards by default or completely hide them for this version.

@MilGard91
Copy link
Contributor

Hi @alxs, regarding

  • No support for identicon

@polkadot/vue-identicon currently doesn't support vue 3 and can't be implemented in the project, we are currently using gravatar or icon from the figma. I'm open to other solutions if you have a suggestion.

@alxs
Copy link
Contributor

alxs commented Aug 5, 2021

Hey @MilGard91. Is this of any use to you? It's also a current delivery, so feel free to leave feedback directly on that PR if you run into any issues.

Good call on creating an issue for @polkadot/vue-identicon though, I think chances are high this would be fixed pretty soon.

@alxs
Copy link
Contributor

alxs commented Aug 5, 2021

@Jakic007 I would argue that these are part of the deliverables for this milestone. I can see this not being entirely clear in the application, but it's also not stated anywhere that this wouldn't be the case and they're part of both the mock-ups and the delivered designs.

Deliverable 4 simply states 'Build UI components in VueJS used in the individual identity page', the mock-up for which includes the governance and treasury modules.

Under 'Future plans' you mention that you plan on implementing a plug-in system and that 'default plug-ins would be basic info, governance, treasury and validator', which however also doesn't mean these can't already have been implemented with M1 as the basic info clearly needs to be and is listed along the others.

I'd also argue that the price is high enough for this to be part of the deliverables.

@edisinovcic
Copy link

Thanks. A few more remarks:

I would recommend you to go through an extensive review of the project as it currently seems to be broken anywhere I look. I'd like to ask you to please do so and bring this to an acceptable state within 14 days as specified in the terms and conditions, seeing as the delivery is already delayed by over 3 months, or the grant may be terminated.

Should be fixed by now. Can you confirm @alxs ?

@DeFiYaco
Copy link
Contributor Author

@alxs latest version is currently being deployed. I will write when its live.

@DeFiYaco
Copy link
Contributor Author

@alxs latest version is currently being deployed. I will write when its live.

Ready @ https://identityhub.xyz/

Copy link
Contributor

@alxs alxs left a comment

Choose a reason for hiding this comment

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

  • Querying by index or identity fields isn't implemented or doesn't work.
  • Did you have any luck with the library I referred you to in my previous comment for the identicon? Identicons are an important part of one's visual identity in Polkadot/Kusama and it would make a lot of sense to make use of them on a website with just this purpose. I don't know how you planned to implement profile pictures as in your mockups, but you're of course welcome to do that instead. There is an image field in the identity pallet, but I don't think anyone uses it and seeing as it may contain just the hash of an image, it's probably not very useful for this use case.
  • For the record: governance and treasury are part of the required functionality for this milestone as long as the amendment hasn't been approved.
  • When navigating through identities, often one lands on a URL with undefined in place of the network, which is then broken if shared or reloaded.
  • When connecting to the polkadot{.js} extension, all addresses are shown, instead of only the ones pertaining to the current network.
  • Connecting to a custom node via the top-left menu doesn't seem to work.
  • Inline documentation would greatly improve code readability and is required as per deliverable 0b and per our delivery guidelines.
  • It would be nice if you could link to the documentation in the docs folder from the README and briefly describe the purpose of each file, or for the ones that contain just one paragraph simply integrate them into the README.

@DeFiYaco
Copy link
Contributor Author

DeFiYaco commented Aug 27, 2021

Thanks for the feedback, we are going to fix those issues during the next week and redeploy.

  • Query by index will be implemented. What is meant by querying by identity field?
  • We did not have any luck with that library, and we are already fetching profile pictures like in the mock-ups retrieved from the image field. For those identities without profile pictures, we are displaying default picture
  • We are waiting for amendment approval
  • Could you provide an example of broken flow that leads to an undefined place? We couldn't replicate that
  • That issue is present, and we are going to solve it
  • Connecting to custom node works, but list view does not. You can still search for accounts. Keep in mind that you have to provide websocket endpoint. List view will be fixed.
  • Inline documentation will be added

@alxs
Copy link
Contributor

alxs commented Aug 27, 2021

What is meant by querying be index field?

Identity field. Querying by any of the identity fields i.e. email address, display name etc.

we are already fetching profile pictures

Indeed, I just hadn't stumbled upon one that had it set yet. Nice!

provide an example of broken flow

Sure: click on any identity, go back, click on any identity. Note that this is only the URL that is broken as I pointed out.

Let me know when you're done with the changes and I'll have another look.

Btw: you could easily move the search bar to the top and save a lot of space, plus I think the text above it is only confusing since it gives the impression that you can always search for identities on both Polkadot and Kusama. You may want to change this.

@DeFiYaco
Copy link
Contributor Author

Hey @alxs ! We have pushed some fixes for the things you have mentioned and some other bugs we've found while testing. You can see the latest version live at https://identityhub.xyz

Query by indices is implemented now. We are researching the best approach for enabling query by identity fields.

@DeFiYaco
Copy link
Contributor Author

@alxs Query by identity fields (name, email...) is implemented now. It is ready for review

@alxs
Copy link
Contributor

alxs commented Sep 23, 2021

Thanks @Jakic007. A few last things:

And some UX issues:

  • There's no visual feedback when a query is sent, which gives the impression that the page is unresponsive.
  • The text above the search bar still always reads 'Search registered identities on Kusama or Polkadot network' although only one network at a time is supported
  • This hasn't been addressed yet either

    When connecting to the polkadot{.js} extension, all addresses are shown, instead of only the ones pertaining to the current network.

  • The network dropdown makes it seem as if the choice was between Polkadot, Kusama or a custom node, whereas it's (I think) always Polkadot or Kusama with the option to use a custom node. A different design would make more sense here. There's also no feedback when a node for a different network is used instead.
  • Currently the first 3 identities on each page are shown at the top of the list in larger cards, for which there is no apparent reason.

Please try to address these in the coming days as this evaluation has been dragging on for quite a bit.

On a side note, as you may be aware paritytech/substrate#8615 has been merged which you mentioned was blocking you for M3.

@DeFiYaco
Copy link
Contributor Author

DeFiYaco commented Sep 24, 2021

* Links to an account return to the landing page https://identityhub.xyz/polkadot/12dfEn1GycUmHtfEDW3BuQYzsMyUR1PqUPH2pyEikYeUX59o

Thanks - we are fixing that issue

* Querying by display name only works if the full string is provided, which isn't the expected functionality and doesn't even work for most identities since spaces aren't allowed. Could you please implement searching using partial strings?

We are investigating solutions for this problem

And some UX issues:

* There's no visual feedback when a query is sent, which gives the impression that the page is unresponsive.

Loading animation will be added

* The text above the search bar still always reads 'Search registered identities on Kusama or Polkadot network' although only one network at a time is supported

I agree. It is misleading and confusing. We will change that.

* This hasn't been addressed yet either
  > When connecting to the polkadot{.js} extension, all addresses are shown, instead of only the ones pertaining to the current network.

Could you please give us more details for this? We are showing all account addresses formatted for the specific chain: https://wiki.polkadot.network/docs/learn-accounts#address-format

* The network dropdown makes it seem as if the choice was between Polkadot, Kusama or a custom node, whereas it's (I think) always Polkadot or Kusama with the option to use a custom node. A different design would make more sense here. There's also no feedback when a node for a different network is used instead.

It is possible to connect to Westend or some other Substrate node by choosing custom node option. We can retrieve the network name and show it after successful connection.

* Currently the first 3 identities on each page are shown at the top of the list in larger cards, for which there is no apparent reason.

That design decision was in the original proposal. If you think it is better to display all identities as a list, we can do that.

Please try to address these in the coming days as this evaluation has been dragging on for quite a bit.

On a side note, as you may be aware paritytech/substrate#8615 has been merged which you mentioned was blocking you for M3.

Thank you!

@alxs
Copy link
Contributor

alxs commented Sep 27, 2021

Could you please give us more details for this?

Happy to. Accounts on the extension can either be allowed to be used on any chain or only on a specific one. You're displaying all of them, including the ones that are only allowed on other chains.

It is possible to connect to Westend or some other Substrate node by choosing custom node option.

Yeah, I may have blindly tried a few chains that didn't necessarily have the identity pallet. If that's basically the requirement this could be shared with the user when they enter the node, and some feedback given if a chain without it is used.

That design decision was in the original proposal. If you think it is better to display all identities as a list, we can do that.

This looks fine on the first page, but doesn't make a lot of sense on later ones. I'd have expected the list to be detached from the cards.

@DeFiYaco
Copy link
Contributor Author

@alxs Issues have been resolved. Ready to review: https://identityhub.xyz

@alxs
Copy link
Contributor

alxs commented Sep 30, 2021

@Jakic007 still some issues:

  • Partial search doesn't work properly: searching for 'foundation' returns no results.

  • Switching networks after searching should remove the search string and return to the default view (with the 3 cards). The current behaviour is broken as not only is the search box populated without the corresponding results being shown, but trying to run the query doesn't work.

  • Banner still shows 'Search registered identities on Kusama or Polkadot network' when selecting custom node.

  • When the end of the search results has been reached, the message 'There is no more identities' is shown. Suggest 'No more results' instead.

  • Yeah, I may have blindly tried a few chains that didn't necessarily have the identity pallet. If that's basically the requirement this could be shared with the user when they enter the node, and some feedback given if a chain without it is used.

    This doesn't seem to be the requirement as Moonriver and Khala both implement the identity pallet and I couldn't connect to either. Please do let the user know what the requirements are and provide feedback when they aren't met.

  • It is currently impossible to return from a search, and clicking on 'Identity Hub' to the left doesn't work.

  • Switching networks when connected with an account that isn't allowed to be used in the new network should disconnect.

  • Sending tokens on Westend displays DOT as the currency. I wasn't able to test it with other networks as I don't know which work and which don't, but I assume it always defaults to DOT.

  • In some cases, when no website is provided for an identity, the corresponding button leads to the current identity page, while in others it doesn't. This is confusing and I think should be removed altogether (the buttons shouldn't be 'clickable' either if they don't do anything, and there isn't really a reason for them to still be displayed).

@DeFiYaco
Copy link
Contributor Author

DeFiYaco commented Oct 5, 2021

@alxs Issues are resolved. Feel free to review: https://identityhub.xyz

@alxs
Copy link
Contributor

alxs commented Oct 5, 2021

Search still seems to return some erroneous results - now searching for 'foundation' also returns this account, which doesn't contain the word anywhere in its identity.

I also just realised the reason the polkadot{.js} extension wouldn't decode some transactions: it needs to update the chain metadata. This is an issue quite a few people will experience and won't be able to figure out for themselves. I think the most sensible fix would be to prompt the user to do so if necessary upon clicking 'Connect'. In the same spirit, there should be some feedback as to why the functionality is disabled if they don't have the extension installed.

Lastly, I noticed that links to an account don't work on Westend since the node URL is lost - you could include it in the URL alike Polkadot.js Apps, but this isn't required for this milestone.

@MilGard91
Copy link
Contributor

Hi @alxs, regarding the metadata update, can you tell me where do you want us to point them to so they can update it? I will add the note to the connect modal, so they will see it when they pick the wallet.

@alxs
Copy link
Contributor

alxs commented Oct 7, 2021

@MilGard91 I've always seen this implemented within the dapp/website, I assume it shouldn't be hard to accomplish. You can check if the extension has the latest metadata and otherwise prompt the user to update it.

@DeFiYaco
Copy link
Contributor Author

Search still seems to return some erroneous results - now searching for 'foundation' also returns this account, which doesn't contain the word anywhere in its identity.

There was actually 'foundation' string under the display name (BIFROST FOUNDATION) field that was hidden before. That's why it returned that result. Now, we are showing the display name under every identity.

I also just realised the reason the polkadot{.js} extension wouldn't decode some transactions: it needs to update the chain metadata. This is an issue quite a few people will experience and won't be able to figure out for themselves. I think the most sensible fix would be to prompt the user to do so if necessary upon clicking 'Connect'. In the same spirit, there should be some feedback as to why the functionality is disabled if they don't have the extension installed.

This is implemented now. User will see a button if there is need to update metadata once he wants to connect to wallet.

Lastly, I noticed that links to an account don't work on Westend since the node URL is lost - you could include it in the URL alike Polkadot.js Apps, but this isn't required for this milestone.

We have added Westend to the chainpicker now, so those URLs will work. Identity wouldn't be fetched from the custom RPC in that case. But, as you said, it is not required for this milestone.
Note: we are using public RPC endpoint for westend (wss://westend-rpc.polkadot.io). It is not possible to connect with it to Westend because the node is not updated to v0.9.10. We had the same issue with our Polkadot and Kusama nodes.
You can read about that here: https://github.com/paritytech/polkadot/releases/tag/v0.9.10
Once polkadot.io updates that node, it will work.

@DeFiYaco
Copy link
Contributor Author

An issue hash appeared for Polkadot and Kusama chains. We are investigating that and will let you know once it's fixed. @alxs

@DeFiYaco
Copy link
Contributor Author

@alxs Fixed! Feel free to review.

@alxs
Copy link
Contributor

alxs commented Oct 13, 2021

@Jakic007 there seems to be an issue with the registrar verifications (and a typo in the word 'registrar'):

image

@alxs
Copy link
Contributor

alxs commented Oct 13, 2021

That's why it returned that result. Now, we are showing the display name under every identity.

Got it. Shouldn't the display name be used as the display name?

User will see a button if there is need to update metadata once he wants to connect to wallet.

I didn't get any prompt or see a button. Is this live?

@DeFiYaco
Copy link
Contributor Author

Got it. Shouldn't the display name be used as the display name?

Currently, we are displaying legal name in the list. Neither display name nor legal name are required fields, so we first try to display legal name if it exists, then display name, and if none of those exist, we display the address. Of course, that can be switched. What do you suggest?

I didn't get any prompt or see a button. Is this live?

Yes, it's live. Maybe your metadata is already up-to-date? You can check that here: https://polkadot.js.org/apps/#/settings/metadata
In that case, you will not see a button. If you want to test, you can install polkadotjs extension in another browser and check from there. It should have older metadata, and it should prompt you to update.
Keep in mind that the metadata update depends on the chain. You can maybe have an update available for Westend and not for Polkadot.

@alxs
Copy link
Contributor

alxs commented Oct 13, 2021

I see, I suggest using the display name as the default as that would be its intended usage.

My metadata is outdated:
image

@DeFiYaco
Copy link
Contributor Author

We are now using the display name as the default.
Please check if you can see the metadata update button now? Also, which browser are you using? @alxs
image

@alxs
Copy link
Contributor

alxs commented Oct 14, 2021

That worked, I only noticed the way the button disappears once clicked is quite glitchy and doesn't actually check that the metadata has been updated. I'm using Chrome 94.0.4606.61.

My last point would be performance: the website is currently very slow to load with the loading time seemingly proportional to the number of identities on a given chain. For Kusama, I measured over 20s only to load the landing page. Seeing as the number of identities can only possibly grow larger, this is a rather unfortunate approach. Besides, there is naturally no need to load all identities on start. Could you find a more sensible solution?

@DeFiYaco
Copy link
Contributor Author

Our stance is that we have delivered all the promised deliverables for the Milestone 1. There were bugs along the way, all of those were fixed, as it is visible in this PR's comments. There will always be space for optimizations and polishing and those will be upgraded as the product progresses and we work on it. We would like if we could conclude this delivery with this commit: Shard-Labs/identity-hub@781b9f2

@alxs
Copy link
Contributor

alxs commented Oct 18, 2021

@Jakic007 fair enough, the milestone is a pass. You can find my evaluation notes here and as usual I'll forward your invoice for processing. I hope you can find a way to reduce the loading time soon as I really feel this significantly impacts usability.

@alxs alxs merged commit 13d0d30 into w3f:master Oct 18, 2021
@RouvenP
Copy link

RouvenP commented Oct 22, 2021

hi @Jakic007 we transferred the payment today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants