-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: use registered API keys in Candid-RPC methods #90
Conversation
json, | ||
max_response_bytes, | ||
async fn http_request( | ||
_provider: &RpcNodeProvider, |
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.
Why is the provider ignored?
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.
The provider
param will eventually be used for logging. For context, the http_request()
method was originally added to enable mock HTTP outcalls in unit tests, but I removed this in favor of using state machine tests.
let providers = providers.borrow(); | ||
Some( | ||
providers | ||
.iter() |
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.
Wouldn't it be more efficient to use a hash map rather than iterating through the whole list (possibly twice)?
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.
Currently feels like a premature optimization (expecting maybe 10 providers overall), but it would make sense to eventually use a HashMap. I will definitely keep this in mind.
ETH_MAINNET_CHAIN_ID, | ||
match provider { | ||
EthereumProvider::Ankr => "rpc.ankr.com", | ||
EthereumProvider::BlockPi => "ethereum.blockpi.network", |
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.
FYI @rvanasa : We are going to kick out BlockPi on the used providers for ckETH because it (without API keys) it has a hard limit of 25kB on the response size of any request that was problematic for us (eth_getLogs
)
This PR changes the Candid-RPC methods to use registered
RpcProvider
API keys (previously only used for generic JSON-RPC requests).Resolves #73.