Add service keys for evm currencies #946
Conversation
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. 🔥
src/ethereum/ethereumTypes.ts
Outdated
| quiknodeApiKey?: string | ||
| serviceKeys?: Record<string, string | string[]> |
There was a problem hiding this comment.
Let's go with serviceKeys: { [host: string]: string[] } as the type. Using this type annotation style allows us to document the key with a name for semantics, and also we want it to always be an array which supports single and multiple items just as well.
There was a problem hiding this comment.
This is still not addressed.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
e3c5453 to
35d8645
Compare
|
|
||
| return await this.serialServers(async baseUrl => { | ||
| const url = `${base58ToHexAddress}${path}` |
There was a problem hiding this comment.
@samholmes So, was this a bug?
There was a problem hiding this comment.
I don't know what happened here. Though I notice that fetchGetAmberdataApi is dead code, so maybe this just needs to be deleted. I think we're no longer using Amberdata altogether, so maybe this entire adapter should be removed so we cleanup tech-debt. Though, I don't know for sure if we want to make that move before we re-support Amberdata as an adapter.
There was a problem hiding this comment.
Answer: Amberdata and Blockcypher adapters are deprecated and will be removed in a follow-up PR/task.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
35d8645 to
df65d34
Compare
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.
|
|
||
| return await this.serialServers(async baseUrl => { | ||
| const url = `${base58ToHexAddress}${path}` |
There was a problem hiding this comment.
Answer: Amberdata and Blockcypher adapters are deprecated and will be removed in a follow-up PR/task.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
df65d34 to
31b8831
Compare
src/common/serviceKeys.ts
Outdated
| export function getServiceKeyIndex(url: string): string { | ||
| const host = new URL(url, `scheme://${url}`).host | ||
| return host | ||
| } |
There was a problem hiding this comment.
@samholmes This handles a host being passed in now
src/common/serviceKeys.ts
Outdated
| @@ -11,6 +11,6 @@ export function getRandomServiceKey( | |||
| } | |||
|
|
|||
| export function getServiceKeyIndex(url: string): string { | |||
| const host = new URL(url).host | |||
| const host = new URL(url, `scheme://${url}`).host | |||
There was a problem hiding this comment.
Use https as the default scheme that is assumed. Also make this commit a "fixup!" commit to the relevant commit where getServiceKeyIndex was introduced.
tomatrow
force-pushed
the
aj/add-service-keys-for-evm-currencies
branch
from
bdf7b9a to
5716bf6
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none