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.

Sign up for GitHub

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

Jump to bottom

fix: /accounts/{accountId}/convert encoding for ecdsa #1280

Merged
merged 3 commits into from
Jun 29, 2023
Merged

fix: /accounts/{accountId}/convert encoding for ecdsa #1280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e3969b
start the rework
TarikGul May 3, 2023
946d0a8
Merge branch 'master' of github.com:paritytech/substrate-api-sidecar …
TarikGul Jun 29, 2023
b975a52
Update comment in tests
TarikGul Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/services/accounts/AccountsConvertService.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,29 @@ describe('Convert accounts', () => {
).toStrictEqual(expectedResponse);
});

// This ensures the behaviour of the endpoint correctly converts a kusama publicKey given
// the following input. See PR: https://github.com/paritytech/substrate-api-sidecar/pull/1280
it('Should convert a valid Kusama publicKey when `publicKey` equals `true`', () => {
const expectedResponse = {
accountId:
'0x96074594cccf1cd185fa8a72ceaeefd86648f8d45514f3ce33c31bdd07e4655d',
address: 'Fy2rsYCoowQBtuFXqLE65ehAY9T6KWcGiNCQAyPDCkfpm4s',
network: 'kusama',
publicKey: true,
scheme: 'sr25519',
ss58Prefix: '2',
};

const kusamaPublicKey =
'0x96074594cccf1cd185fa8a72ceaeefd86648f8d45514f3ce33c31bdd07e4655d';

expect(
sanitizeNumbers(
validateService.accountConvert(kusamaPublicKey, 'sr25519', 2, true)
)
).toStrictEqual(expectedResponse);
});

// We try to convert a Polkadot AccountId to an SS58 Address by setting the publicKey=true
// which is not correct and that is why in the response we have an invalid address.
// If we would like to convert it correctly and have the expected SS58 address
Expand All @@ -158,7 +181,7 @@ describe('Convert accounts', () => {
const expectedResponse = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Old Code
By reading the comments above this test, it was giving as output an invalid address on purpose.
The comments say that the user :

  • gives to the endpoint a non public key but
  • says/states falsely to the endpoint that it is a public key (by setting query param publicKey=true)
    and then the output is/should be an invalid address. So that was with the old code.

New Code
With the new code, the output is correct even if the user states that the input is a public key when its not. So in the case of scheme=ecdsa, if the user sets query param publicKey either true or false
it will go to this new line of code :
ecdsa: (p: string) => (hexToU8a(p).length > 32 ? blake2AsHex(p) : p),
and it will encode correctly despite the value of publicKey query param.

Thoughts
So, some thoughts based on the above :

  • In retrospective, since I was the one who added the original test, I have to admit that it is a little bit confusing and not sure how useful it is anymore. I think my goal was to "catch" all possible combinations of inputs.
  • However, if we keep this test with the new code, I think we should change the comments so that they reflect what the test is doing right now.
  • Also, by checking the new code, I am not sure if the query param publicKey is needed anymore. Wdyt @TarikGul ? Since it will convert it correctly by checking the length (in the ecdsa case)

cc @IkerAlus Thank you for your time on trying to check this. It really helped on clearing my head and checking efficiently after our talk.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if the query param publicKey is needed anymore. Wdyt @TarikGul ? Since it will convert it correctly by checking the length (in the ecdsa case)

I agree that the publicKey query param is not needed anymore. I would say we keep this PR small to first fix the way we deal with ecdsa then we can have some follow up work to remove the publicKey query param.

However, if we keep this test with the new code, I think we should change the comments so that they reflect what the test is doing right now.

👍 Sounds good.

ss58Prefix: '0',
network: 'polkadot',
address: '12ZviSbX1Pzmnw1mg4FUg6Qra2CW7Q3z9iqmcWKWUphp5qgq',
address: '1rsCBWhPgyDETNS9yxnANSnm3KAtkxm4mu9jjfMhDF6xaV8',
accountId:
'0x2607fd20388303bd409e551202ee47b753b4382feac914e9f7ab0d4f728c2bf7',
scheme: 'ecdsa',
Expand Down
20 changes: 17 additions & 3 deletions src/services/accounts/AccountsConvertService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,25 @@

import { Keyring } from '@polkadot/api';
import { isHex } from '@polkadot/util';
import { allNetworks } from '@polkadot/util-crypto';
import { blake2AsHex } from '@polkadot/util-crypto';
import { hexToU8a } from '@polkadot/util';
import { allNetworks, blake2AsHex } from '@polkadot/util-crypto';
import { BadRequest } from 'http-errors';

import { IAccountConvert } from '../../types/responses/AccountConvert';
import { AbstractService } from '../AbstractService';

/**
* Copyright 2023 via polkadot-js/common
*
* The slightly modified below logic is copyrighted from polkadot-js/common . The exact path to the code can be seen here:
* https://github.com/polkadot-js/common/blob/e5cb0ba2b4a6b5817626cc964b4f66334f2410e4/packages/keyring/src/pair/index.ts#L44-L49
*/
const TYPE_ADDRESS = {
ecdsa: (p: string) => (hexToU8a(p).length > 32 ? blake2AsHex(p) : p),
ed25519: (p: string) => p,
sr25519: (p: string) => p,
};

export class AccountsConvertService extends AbstractService {
/**
* Takes a given AccountId or Public Key (hex) and converts it to an SS58 address.
Expand Down Expand Up @@ -59,7 +71,9 @@ export class AccountsConvertService extends AbstractService {
);
}

const accountId2Encode = publicKey ? blake2AsHex(accountId) : accountId;
const accountId2Encode = publicKey
? TYPE_ADDRESS[scheme](accountId)
: accountId;

const keyring = new Keyring({ type: scheme, ss58Format: ss58Prefix });
const address = keyring.encodeAddress(accountId2Encode, ss58Prefix);
Expand Down