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

Address verification enhancement #724

Closed
wants to merge 1 commit into from

Conversation

toolsopen
Copy link

No description provided.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 19, 2021

User @toolsopen, please sign the CLA here.

@TarikGul
Copy link
Member

@toolsopen Thanks for the PR. If you could give a description as to why you think this is a verification enhancement, and also some tests to support it.

@@ -56,7 +57,9 @@ async function main() {
types: TYPES ? (require(TYPES) as RegistryTypes) : undefined,
/* eslint-enable @typescript-eslint/no-var-requires */
});

if(api.registry.chainSS58 != undefined){
Copy link
Member

Choose a reason for hiding this comment

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

So this logic would actually have to be contained inside of the middleware. It would have to check if the requests are grabbing information from a past block that has a historic runtime OR if there is no at query parameter attached then to just use the current api's registry. This also stops us from mutating any data in polkadot-js through { defaults }, and potentially having other files import that mutated dep.

Copy link
Contributor

Choose a reason for hiding this comment

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

chainss58 doesn't change so I don't think we need to worry about historical queries

Copy link
Contributor

Choose a reason for hiding this comment

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

(not sure if I am understanding correctly your comment tho)

Copy link
Member

Choose a reason for hiding this comment

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

Yea I see what you're saying. Yea if chainss58 never changes one could use the current registry.

const [isValid] = checkAddressChecksum(u8Address);

const [isValid, , , ss58Decoded] = checkAddressChecksum(u8Address);
if (ss58Decoded !== prefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO its a feature not a bug that we will accept any address format. If we want this level of address validation we can add an address validation endpoint

Copy link
Author

Choose a reason for hiding this comment

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

Yes, my purpose is just to verify the address

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.

I don't think we should do address validation based on the prefix. If we want that level of checks we could add an endpoint that tells some info about an address, like what network its associated with and generally if its valid

@toolsopen
Copy link
Author

#726

@toolsopen toolsopen closed this Oct 28, 2021
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.

4 participants