Add context.rpc #801
Add context.rpc #801
Conversation
|
@morgsmccauley kindly take it over from here in case I did not wire things properly in the streamer or some other place |
| describe('RPCClient unit tests', () => { | ||
| const rpcClient = RpcClient.fromConfig({ | ||
| networkId: 'mainnet', | ||
| nodeUrl: 'https://beta.rpc.mainnet.near.org', |
There was a problem hiding this comment.
Can we add tests which don't rely on real endpoints? I don't want unnecessary CI failures due to network failures.
|
|
||
| rpcClient = RpcClient.fromConfig({ | ||
| networkId: 'mainnet', | ||
| nodeUrl: 'https://beta.rpc.mainnet.near.org', |
There was a problem hiding this comment.
Similar comment around adding tests which don't depend on real infrastructure. This isn't being used in integration tests so I would just remove it.
| @@ -96,7 +97,9 @@ async function blockQueueProducer (workerContext: WorkerContext): Promise<void> | |||
| async function blockQueueConsumer (workerContext: WorkerContext): Promise<void> { | |||
| let previousError: string = ''; | |||
| const indexerConfig: IndexerConfig = workerContext.indexerConfig; | |||
| const indexer = new Indexer(indexerConfig); | |||
| const indexer = new Indexer(indexerConfig, { | |||
| rpcClient: RpcClient.fromEnv() | |||
There was a problem hiding this comment.
We typically add defaults for injected dependencies to add convenience, could you do that please?
There was a problem hiding this comment.
I was actually proposing to not have defaults to make configuration of this class explicit with all dependencies – the number of places it is being configured in is very low. This will also be needed for the testing framework. Happy to discuss or change my opinion if defaults indeed make things more convenient.
There was a problem hiding this comment.
What benefit would the explicitness provide? For me, defaults allow us to abstract the implementation away, so therefore don't need to think about it from the user side. I hadn't really considered otherwise, interested to see your side.
| return new RpcClient(config); | ||
| } | ||
|
|
||
| static fromEnv (): IRpcClient { |
| } | ||
|
|
||
| static fromEnv (): IRpcClient { | ||
| if (!process.env.RPC_URL) { |
There was a problem hiding this comment.
| if (!process.env.RPC_URL) { | |
| if (!process.env.NEAR_RPC_ENDPOINT) { |
I've deployed under this environment variable, I didn't want it to be confused with the other RPC endpoints we use internally.
There was a problem hiding this comment.
That actually makes sense. I was thinking to actually name it 'ARCHIVAL_RPC_ENDPOINT' – do you want me to make this change in this PR?
There was a problem hiding this comment.
Sure, I've deployed NEAR_RPC_ENDPOINT, but can update.
| @@ -63,6 +63,7 @@ | |||
| "express": "^4.18.2", | |||
| "graphql": "^16.8.1", | |||
| "long": "^5.2.3", | |||
| "near-api-js": "^4.0.2", | |||
There was a problem hiding this comment.
We should be using @near-js packages instead, so for this I think that would mean @near-js/types and @near-js/providers.
near-api-js includes much more than we need, and @near-js is more likely to be maintained going forward.
|
|
||
| interface Dependencies { | ||
| fetch: typeof fetch | ||
| provisioner: Provisioner | ||
| dmlHandler?: DmlHandler | ||
| indexerMeta?: IndexerMeta | ||
| parser: Parser | ||
| }; | ||
| rpcClient?: IRpcClient |
There was a problem hiding this comment.
| rpcClient?: IRpcClient | |
| rpcClient: IRpcClient |
I'm not sure there is any reason not to include this? dmlHandler etc is only optional because we need to get DB credentials.
This would also tidy up the context generation since it would always be defined.
If these are all comments, I'll make necessary fixes and let you pick it up from there |
|
@pkudinov @morgsmccauley Sorry for jumping in, but I'd like to invite you to participate in the Race of Sloths - a fun and gamified open-source contributions program. Consider mentioning @race-of-sloths user in your github comment or PR description to join! See how the flow works here: near/nearcore#11778 |
Implements #797
It also implements a way to configure dependencies through static methods (e.g.
RpcClient.fromEnv()toIndexerthat I recommend to adopt.When deploying to dev or produciton, we need to ensure that we use a Read RPC endpoint, and not simply an archival node.