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: add /accounts/:accountId/convert endpoint #1007

Merged
merged 11 commits into from
Sep 21, 2022
Merged

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Aug 3, 2022

Description

This endpoint will receive as an input an Account ID or a Public key (hex) that is given by the user as the path parameter and convert it into an SS58 address. The conversion will depend on the values of the 3 query parameters that are mentioned below.

This implementation was motivated by this comment while making changes in the validate endpoint.

From SS58 Address to Account ID also ?
Does this endpoint also implements the reverse conversion, meaning from SS58 Address to Account ID ? No because this functionality is already covered by the /accounts/:address/validate endpoint after this PR was merged.

Query Parameters

This endpoint can take the following 3 query parameters :

  • scheme
  • prefix
  • publicKey

Possible values

The possible values for these parameters are the following :

  • scheme can take one of those 3 values {sr25519, ed25519, ecdsa}
  • prefix can be one of the prefices found here
  • publicKey is a boolean value and can be true or false

Default values

The default values for these query params are the following :

  • scheme = sr25519
  • prefix = 42
  • publicKey = true

Examples

  • So, for example if the query parameters that are given are the following :

    • prefix=2
    • scheme=ed25519
    • publicKey=false
  • then that means that the endpoint will give as an output :

    • a Kusama formatted SS58 address (based on the ss58-registry the prefix=2 corresponds to the Kusama network)
    • based on the ed25519 algorithm (default value)
    • assuming that the given path parameter is an Account ID (and not a Public Key (hex))
  • If no query parameters are specified then the endpoint will take their default values (as defined above) and will output :

    • a Substrate formatted SS58 address (default value of prefix = 42 which corresponds to substrate)
    • based on the sr25519 scheme (default value) and
    • assuming that the given path parameter is a Public Key (hex)

Special Case / Use of query param publicKey

In most of the cases we can output a valid SS58 address based only on the scheme and prefix since Public Key (hex) is identical to the Account ID (so specifying the query param publicKey does not make any difference).

However, there is a specific case for which we also need to know if the path parameter that is given is :

  • a Public Key (hex) or
  • an Account ID

hence we need the query parameter publicKey to be specified as true or false.

This case is when we have scheme=ecdsa.
When we have ecdsa as scheme then Public key is different than the Account ID so we first need to hash the public key with blake2 and then get the SS58 address from that.

*** This is also explained in this substrate PR (subkey: display SS58 encoding of public key)

Sample Response

Sample request :
/accounts/0x002b5bab9258f5aff3117fce47854ef4c29c0eeae3faf855a2d6a9b67861c598/validate?prefix=0

Sample response :

{
  "ss58Prefix": "42",
  "network": "substrate",
  "address": "5C4vjiCiAiVYcx5fZ9g9FftViRqRwFRNpSDVrh6dzE35x2d8",
  "accountId": "0x002b5bab9258f5aff3117fce47854ef4c29c0eeae3faf855a2d6a9b67861c598",
  "scheme": "sr25519",
  "publicKey": true
}

chains-config

It was also added in chains-config -> Controllers of :

  • Polkadot
  • Kusama
  • Westend

Todos

  • Double check if the conversion (from accountId to address) that happens in the accountConvert method is correct
  • Make use of the query param public (because currently is not used)
  • Update Docs
  • Add Tests

- Controller, service, response type
@Imod7 Imod7 requested a review from a team as a code owner August 3, 2022 06:47
@jsdw
Copy link
Collaborator

jsdw commented Aug 3, 2022

Dumb question probably (I read the comments this came from and didn't quite understand); what does the public query param do? Does it apply to every scheme?

- Using the query param `publicKey` to output the SS58 address if the input/parameter that is given by the user is a Public Key (hex) and not an accountID.
@Imod7
Copy link
Contributor Author

Imod7 commented Aug 16, 2022

Dumb question probably (I read the comments this came from and didn't quite understand); what does the public query param do? Does it apply to every scheme?

@jsdw Thank you so much for your question and it is a quite logical question to have since when I created this PR I was actually not even using the query param public (now renamed to publicKey). After a clarification from @IkerAlus it was also clear to me the purpose of this parameter and now I added some code that actually uses it.
Since this is one very important point/specific case of the implementation of this endpoint, I tried to explain it in the Description of the PR under the section "Special Case / Use of query param publicKey". I hope this explanation and the substrate PR (subkey: display SS58 encoding of public key) that I cite answers your question.

- Added specific types that the `RequestHandler` can accept.
- Cleaned the code that validates the query params (and sets default values) in the Controller.
- Added tests with different valid or invalid endpoints to check.
- Updated the docs with the `convert` endpoint functionality &  path & query params.
@Imod7 Imod7 requested a review from TarikGul August 24, 2022 11:16
@Imod7
Copy link
Contributor Author

Imod7 commented Aug 24, 2022

I added the tests + docs in this PR.

  1. In the docs I populated some "keys" at my own discretion since I am not sure if we follow a pattern/rule to populate those. These are the following :

    • the schema at the "200" response
      schema:
         $ref: '#/components/schemas/AccountConvert'
      
    • the operationId
    • Added 3 responses codes (200, 400, 404)
  2. Another point is that in src/chains-configI only added the AccountConvert controller in Polkadot, Kusama and Westend. Should I add it in all the chains ?

@TarikGul
Copy link
Member

  1. Nice job, sounds good.

Another point is that in src/chains-configI only added the AccountConvert controller in Polkadot, Kusama and Westend. Should I add it in all the chains ?

  1. No need to add it to the others, they can always push up a PR if they would like it.

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
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.

Overall very solid job, just some nits, and a few questions above.

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 Job, LGTM 👍 Just need to resolve the merge conflict

@Imod7 Imod7 requested a review from a team as a code owner September 5, 2022 18:04
@TarikGul TarikGul removed the request for review from a team September 6, 2022 17:54
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.

New commits look great! Lets get this merged in soon.

Comment on lines +46 to +50
!(
cryptoScheme === 'ed25519' ||
cryptoScheme === 'sr25519' ||
cryptoScheme === 'ecdsa'
)
Copy link
Collaborator

@jsdw jsdw Sep 21, 2022

Choose a reason for hiding this comment

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

Totally happy with this!

When there are lots of conditions you can also use an approach like if (!['ed25519', 'sr25519', 'ecdsa'].includes(cryptoScheme)) { ... }, but your way is more efficient :)

Comment on lines +49 to +55
let network = null;
for (const networkParams of allNetworks) {
if (networkParams['prefix'] === ss58Prefix) {
network = networkParams['network'];
break;
}
}
Copy link
Collaborator

@jsdw jsdw Sep 21, 2022

Choose a reason for hiding this comment

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

Assuming allNetworks is an array, you can also do:

let network = allNetworks.find(n => n['prefix'] === ss58Prefix);
if (!network) {
   throw new BadRequest(...)
}

Not a big deal though :)

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.

lgtm, nice work!

@Imod7 Imod7 merged commit e2d6fae into master Sep 21, 2022
@Imod7 Imod7 deleted the domi-convert-endpoint branch September 21, 2022 11:13
This pull request was closed.
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.

3 participants