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

[Wallet] Fix 8 digit codes for mainnet #6663

Merged
merged 4 commits into from
Jan 26, 2021
Merged

Conversation

i1skn
Copy link
Contributor

@i1skn i1skn commented Jan 25, 2021

Description

  • Fix attestaion service URL, when / in the end is missing
  • Remove firebase proxies.

Tested

Verification flow on Android device

Backwards compatibility

Yes

@i1skn i1skn requested a review from a team January 25, 2021 17:31
@i1skn i1skn force-pushed the i1skn/fix-8-digit-codes branch from cb3b4b3 to 14b2536 Compare January 25, 2021 17:41
@i1skn i1skn requested review from asaj and timmoreton as code owners January 25, 2021 17:41
Copy link
Contributor

@gnardini gnardini left a comment

Choose a reason for hiding this comment

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

Tested locally on mainnet and is working for me 😄

@nambrot
Copy link
Contributor

nambrot commented Jan 26, 2021

Would note that Mainnet isn't the best testing ground for this as most validators haven't updated yet. So I'd probably recommend doing it on Alfajores as well

@i1skn
Copy link
Contributor Author

i1skn commented Jan 26, 2021

@nambrot this PR specifically fix 8 digit codes on mainnet, on alfajores it works fine without this PR. Also, all validators on mainnet are on >= 1.1.0 version now.

@i1skn i1skn added the automerge Have PR merge automatically when checks pass label Jan 26, 2021
Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Thanks! 👍
Nice seeing one less dependency.

Comment on lines +334 to +338
const resp = await fetch(
`${attestationServiceURLClaim.url}${
attestationServiceURLClaim.url.substr(-1) === '/' ? '' : '/'
}status`
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad JavaScript doesn't provide a builtin way to join url parts.

We do have a string helper though which does the same as what you did:

export function appendPath(baseUrl: string, path: string) {
const lastChar = baseUrl[baseUrl.length - 1]
if (lastChar === '/') {
return baseUrl + path
}
return baseUrl + '/' + path
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@mergify mergify bot merged commit f2bbba6 into master Jan 26, 2021
@mergify mergify bot deleted the i1skn/fix-8-digit-codes branch January 26, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants