Add service keys for evm currencies #946
Conversation
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
Nice PR! Thanks for giving this a stab.
I'd recommend squashing the wip commit with the first commit since it makes sense as one atomic change together.
I'd also recommend setting up your editor to catch ESLint errors for you following the project's rules. Some of the requested changes in the PR would have been caught by tooling which is why it's recommended to use a good integrated linter/type-checker.
I can't wait to test this out and give it a go once the PR fixups are in place. 🔥
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
e3c5453 to
35d8645
Compare
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
I haven't fully finished my re-review. But here's some immediate feedback.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
35d8645 to
df65d34
Compare
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
Almost there. Pull my fixup commit on the branch of the same name on this repo.
src/common/getServiceKeyIndex.ts
Outdated
| export function getServiceKeyIndex(url: string): string | undefined { | ||
| try { | ||
| return new URL(url).host | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
.host is never undefined so omit it in the return type.
I see that you're using undefined as a case at the call-site. It seems like what you want is a utility that does a lookup and returns a string or undefined if the lookup succeeds or fails. I made a fixup commit to aj/add-service-keys-for-evm-currencies branch which you can pull from that has the suggested solution.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
df65d34 to
31b8831
Compare
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
bdf7b9a to
5716bf6
Compare
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
I think this is ready to go. There just is conflicts needing resolved with a rebase. I'll double check the PR after the conflicts are resolved before merging. I'll also run some tests.
src/common/serviceKeys.ts
Outdated
| const host = new URL(url, `https://${url}`).host | ||
| return host |
There was a problem hiding this comment.
While you're at the rebase. Feel free to change this to:
const url = = new URL(url, `https://${url}`)
return url.hostJust so that way we can easily insert a console.log(url) at a later point for debugging.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
2 times, most recently
from
d515a1e to
fe34bd9
Compare
samholmes
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
fe34bd9 to
f1daf49
Compare
samholmes
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
f1daf49 to
60931c5
Compare
samholmes
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
60931c5 to
80e245e
Compare
|
Resubmitted PR: #1005 The reason is because the PR has gotten stale is now being picked up in a new PR to be tested on the latest master branch and to be published. |
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none