Conversation
NikolasHaimerl
changed the title
lib/miner-info.js
Outdated
| return ( | ||
| await getIndexProviderPeerId(minerId, smartContractClient, { | ||
| rpcUrl: RPC_URL, | ||
| rpcAuth: RPC_AUTH, | ||
| maxAttempts | ||
| }) |
There was a problem hiding this comment.
This line is doing too much. Let's split it into two lines to make the code easier to read.
const { peerId } = await getIndexProviderPeerId(minerId, smartContractClient, {
rpcUrl: RPC_URL,
rpcAuth: RPC_AUTH,
maxAttempts
})
return peerId
|
@NikolasHaimerl I have added the missing github workflows, would you mind merging main branch to your PR? |
|
Let's wait until CheckerNetwork/spark-checker#129 is landed first, and then apply the same changes to this repository. |
The PR is ready for review now as CheckerNetwork/spark-checker#129 is merged. |
| const err = new Error(body.error.message) | ||
| err.name = 'FilecoinRpcError' | ||
| err.code = body.code | ||
| console.error(err) |
There was a problem hiding this comment.
the caller of the function should log. With that removed, we can remove the try/catch altogether.
There was a problem hiding this comment.
is the use case scenario here different to CheckerNetwork/spark-checker#129 (comment) ?
There was a problem hiding this comment.
No I don't think so. It also seems to me that your commit wasn't yet approved by Miro, right?
My understanding is that it makes sense to wrap a function body in try/catch if you want to modify the error to be consistent, and then rethrow it. Here we're just logging the error and then throwing it. That's not providing any extra value, so should be removed.
There was a problem hiding this comment.
I approved CheckerNetwork/spark-checker#129, it was already landed.
I agree we may not need the try/catch block here, but I consider it a detail I can live with.
There was a problem hiding this comment.
@juliangruber do you want a change here or should we keep the code in sync with spark-checker?
There was a problem hiding this comment.
Up to you, I would prefer to change here and in spark-checker, but don't want to delay the work stream any further. If we keep this as is, I'm sure someone will eventually refactor it.
| const err = new Error(body.error.message) | ||
| err.name = 'FilecoinRpcError' | ||
| err.code = body.code | ||
| console.error(err) |
There was a problem hiding this comment.
I approved CheckerNetwork/spark-checker#129, it was already landed.
I agree we may not need the try/catch block here, but I consider it a detail I can live with.
4 participants
This PR uses the newly released library to fetch index provider peer ids.
Related to
CheckerNetwork/index-provider-peer-id#10
CheckerNetwork/roadmap#250
CheckerNetwork/roadmap#244