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

feat: Added the network and accountId to the response of validate endpoint #926

Merged
merged 13 commits into from
Jul 5, 2022

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented May 23, 2022

Description

Added the network and accountId in the response from the /accounts/:address/validateendpoint when the given address isValid. If it is an invalid address (isValid: false) then network and accountId have null values.

Functionality of the endpoint

Below you can find a description of how this endpoint behaves and why.

This endpoint is meant to receive only valid addresses and these can be in the following formats :

  1. ss58 format
    • example address from tests 1xN1Q5eKQmS5AzASdjt6R6sHF76611vKR4PFpFjy1kXau4m
  2. hex (from a u8 array) format (of valid length)
    • example address from tests 0x002a39366f6620a6c2e2fed5990a3d419e6a19dd127fc7a50b515cf17e2dc5cc592312
  3. for anything else, this endpoint will/should return null which means isValid field will be false and all other fields will be null.

Comparing with subkey functionality

If we compare this endpoint's functionality with how subkey works then we have the following behaviours :

  1. For the first format : subkey and validate endpoint will return a result :
    • hence will consider this address valid and
    • the AccountId returned will be the same as the one returned from the validate endpoint
  2. For the second format :
    • subkey :
      1. considers this address invalid
      2. but if we correct manually the address and we remove prefix+checksum then it will return a result and the expected AccountId
    • validate endpoint :
      1. considers this address valid. Why ? because it is possible that a user stores their keys in this format.
      2. then strips the prefix+checksum and then returns the correct AccountId

Important Note

If we give as input the address (stripped from prefix+checksum hence AccountId) :

  • to subkey : it will return a valid result
    • why ? because accepts addresses + account ids.
  • to validate endpoint : it will return null
    • why? because this is an AccountId and this endpoint is meant to validate only addresses. For account ids we intend to create another endpoint.

@TarikGul
Copy link
Member

For the moonbeam address I will need to look into that, and play with it. Not exactly sure whats happening there.

Note: Once were satisfied with the changes to the endpoint we'll need to update the docs.

@joepetrowski
Copy link
Collaborator

For the moonbeam address I will need to look into that, and play with it. Not exactly sure whats happening there.

For Ethereum style addresses they are not SS58 formatted, they are just 20 raw bytes. It should be a variant of enum MultiAddress from Substrate.

@TarikGul
Copy link
Member

For Ethereum style addresses they are not SS58 formatted, they are just 20 raw bytes. It should be a variant of enum MultiAddress from Substrate.

Ahh okay cool, I assumed they were leveraging ethereums aaddress format. Thanks for the info.

@TarikGul
Copy link
Member

As a note as well, when its out of [WIP] to change the squashed commit name (title of the PR) to a conventional commit structure.

@Imod7 Imod7 changed the title Added networkName in the response of validate address endpoint Changes in the validate address endpoint May 24, 2022
- If the address given is not a hex value then it returns the hex value of the related registry type
- If the address given is a hex value then it returns the same as the 'accountId'
- The tests were updated based on this logic
src/services/accounts/AccountsValidateService.ts Outdated Show resolved Hide resolved
src/types/responses/ValidateAddress.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

  • Comments on the added accountId in the response of the endpoint and
  • Thoughts on the implementation related to the conversion of the accountId

@Imod7 Imod7 marked this pull request as ready for review May 24, 2022 16:32
@Imod7 Imod7 requested a review from a team as a code owner May 24, 2022 16:32
@Imod7 Imod7 changed the title Changes in the validate address endpoint Changes in the /accounts/:address/validate endpoint May 24, 2022
@Imod7 Imod7 changed the title Changes in the /accounts/:address/validate endpoint feat: Enhancements related to the /accounts/:address/validate endpoint May 24, 2022
- Replaced `filter` with a `for..of` loop when retrieving the `network`.
- Replaced `new TypeRegistry` with `this.api.registry` when retrieving the `accountID`.
- Renamed `networkName` to `network` so it is aligned with the key naming in the SS58 registry.
- Updated the JSDocs of the function `validateAddress`.
- Added the default mock api into the service in the tests.
- Updated the docs.
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Few nits to resolve, but overall great job! 👍

You will also need to run yarn build:docs to regenerate the docs.

Copy link
Collaborator

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

For the Should give the correct response for a {network} hex value tests, I don't understand how the sanitization should know to which network the hex belongs? Wouldn't you have to pass it a network name or prefix as an arg? The hex value should generally speaking be the same across most networks (exceptions being those that don't use AccountId32: [u8; 32] as their AccountId type).

@TarikGul
Copy link
Member

TarikGul commented Jun 5, 2022

@joepetrowski Our process for converting a hex address goes as such:

  1. convert it to a u8a
  2. If the encodedlLength satisfies one of the acceptable lengths then we can check the addresses checksum.
  3. We then specifically use checkAddresssChecksum which will return the ss58Prefix for us.
  4. If its then valid, Using the given information we can then match that ss58Prefix with its known network.

Hope that clears that up :)

- Changes in code based on Tarik's feedback.
- Run `yarn build:docs`.
- Fix for accountId when the input from user is an address in hex (from u8 array) format.
- Corrected the output of the corresponding tests also.
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Great updates, and changes. Good catch @joepetrowski .

Just seems like there is a conflict with the docs.

git pull origin master
yarn && yarn build:docs

That should sort out the merge conflict

@Imod7 Imod7 changed the title feat: Enhancements related to the /accounts/:address/validate endpoint feat: Added the network and accountId to the response of /accounts/:address/validate endpoint Jun 18, 2022
@Imod7 Imod7 changed the title feat: Added the network and accountId to the response of /accounts/:address/validate endpoint feat: Added the network and accountId to the response of validate endpoint Jun 18, 2022
@Imod7
Copy link
Contributor Author

Imod7 commented Jun 18, 2022

Thank you @TarikGul and @joepetrowski for all the comments and reviews on this PR. Very much appreciated!!!

@jsdw jsdw requested a review from joepetrowski June 20, 2022 09:23
@Imod7 Imod7 requested review from jsdw and TarikGul June 29, 2022 19:41
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Couple of wee comments but LGTM!

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Awesome LGTM 👍 Thank you!

@Imod7 Imod7 merged commit ef513cc into master Jul 5, 2022
@Imod7 Imod7 deleted the domi-changes-to-address-validate branch July 5, 2022 01:00
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.

None yet

5 participants