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/:address/validate endpoint #726

Merged
merged 34 commits into from
Oct 27, 2021
Merged

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Oct 20, 2021

This endpoint will validate the address for the given chain.

Response

/accounts/:address/validate

{
    isValid: boolean;
    ss58: number | null
}
  • isValid: Whether or not this address is valid ss58 address.
  • ss58: The ss58 prefix for the address.

TODO

  • Unit Tests
  • Docs
  • e2e Tests (Only need one test per chain). Should be a follow up PR as well to keep this clean and easily readable.

networkId:
type: string
description: Name of the network the address is associated with
ss58:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change this to ss58Prefix for clarity. SS58 is the name of the address format

Copy link
Contributor

Choose a reason for hiding this comment

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

And specify what happens when an address has no prefix

Copy link
Member Author

Choose a reason for hiding this comment

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

And specify what happens when an address has no prefix

I think any valid base58 will provide a "ss58Prefix" that we plug into checkAddressChecksum,

const ss58Length = (decoded[0] & 0b0100_0000) ? 2 : 1;
// ss58Decoded is the prefix
const ss58Decoded = ss58Length === 1
    ? decoded[0]
    : ((decoded[0] & 0b0011_1111) << 2) | (decoded[1] >> 6) | ((decoded[1] & 0b0011_1111) << 8);

so in this case do you think we should just say if the address is not valid a prefix may still be returned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or if we receive a non valid address we just return null for the ss58Prefix

sanitizeNumbers(validateService.validateAddress(karuraHex))
).toStrictEqual(expectedResponse);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

should have a test for a hex value with no prefix and a ss58 value with no prefix to show it works and what the expected prefix is (null?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is an ss58 address with no prefix just a base58 address?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea so to build on that too, every base58 address will always have a prefix as well

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Just add a couple of the mentioned tests, maybe consider my recommendations for what we consider valid and excluding network id from response, and then I think its good to go

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
src/services/accounts/AccountsValidateService.ts Outdated Show resolved Hide resolved
TarikGul and others added 8 commits October 26, 2021 23:05
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: David <dvdplm@gmail.com>
@TarikGul
Copy link
Member Author

@dvdplm Via this comment I changed the returned error for the catch, and added a test for it.

@dvdplm dvdplm merged commit 77bf8ed into master Oct 27, 2021
@dvdplm dvdplm deleted the tarik-validate-address branch October 27, 2021 10:59
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