-
Notifications
You must be signed in to change notification settings - Fork 190
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
Initial Commit for adding support for web3 providers outside of infur… #46
Conversation
…a. Currently, the project only supports infura web3 providers, and more flexibility is needed since infura has had outages.
aefb466
to
db94b6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you run the integration tests with npm run integ-tests
. Pretty sure this will fail on all networks except mainnet
const jsonRpcProvider = sm.Secret.fromSecretAttributes(this, 'jsonRpcProvider', { | ||
secretCompleteArn: 'arn:aws:secretsmanager:us-east-2:644039819003:secret:jsonRpcProvider-UlSwK2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you create the secret in secrets manager? I don't see it there. Also lets sync tomorrow because it should be the production infura id that you use, so i'll need to share that with you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/Uniswap/routing-api/pull/47/files
Hey reworked and simplified this PR entirely
jsonRpcProviderOverride.forEach((url: string, chainId: ChainId) => { | ||
;(env as any)[`JSON_RPC_PROVIDER_${ID_TO_NETWORK_NAME(chainId).toUpperCase}`] = url | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this used? And we set the same url for each one anyway. I commented below, but I think its best to just pass in the project id and the name of the provider (i.e. infura or alchemy), then compute the url for each chain in code.
const url = process.env.JSON_RPC_PROVIDER! | ||
|
||
log.info({ url: url, id: process.env.PROJECT_ID }, `generated rpc url`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use the same URL for all networks. You need to construct the url depending on the chainId passed in.
I think its best to take the other approach we discussed- create the secret so it has two fields, something like
providerName: 'infura'
and
projectId: ...
. Then pass both those into the lambda and construct the URL using a function like
buildJsonRpcUrl(providerName: string, id: string): string
@@ -19,7 +20,8 @@ export class RoutingAPIStage extends Stage { | |||
scope: Construct, | |||
id: string, | |||
props: StageProps & { | |||
infuraProjectId: string | |||
jsonRpcProvider: string | |||
jsonRpcProviderOverride: Map<ChainId, string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we should remove this, but even if we wanted this map there would be no need to create it at this level and pass it all the way through empty. You can just create it inside routing-lambda-stack.ts
I don't know what URL to use for archival node integ-tests, let's discuss that tomorrow. Here is the reworked PR: https://github.com/Uniswap/routing-api/pull/47/files |
Initial Commit for adding support for web3 providers outside of infura. Currently, the project only supports infura web3 providers, and more flexibility is needed since infura has had outages.
#38