apibara / starknet-react

A collection of React providers and hooks for StarkNet
https://starknet-react.com
MIT License
369 stars 147 forks source link

Custom headers for jsonRpcProvider #325

Closed IvanLudvig closed 1 year ago

IvanLudvig commented 1 year ago

adds functionality to pass headers as arguments to jsonRpcProvider. this is useful as some rpcs require passing the auth token in a header

netlify[bot] commented 1 year ago

Deploy Preview for starknet-react ready!

Name Link
Latest commit 9e4acca35d72eb34d67e7b5e4d5a167091d86280
Latest deploy log https://app.netlify.com/sites/starknet-react/deploys/654a4d9f8f9e1300083a34a9
Deploy Preview https://deploy-preview-325--starknet-react.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

fracek commented 1 year ago

I think this PR shows a flaw in the current design. IMHO I should change StarknetConfig.providers to be either a mapping between chainId and a JsonRpcProvider or a function that takes a chain and returns the JsonRpcProvider.

The rough idea:

Then users can use this new providers in the following way:

Option 1: provide a ChainProviderFactory

import { publicProvider } from "@starknet-react/core";

const providers = publicProvider()

Option 2: explicit mapping between chain.id and provider. Using builtin providers this way is ugly but possible.

import { mainnet, goerli } from "@starknet-react/chains";

const providers = {
  [mainnet.id]: myPaidProvider({ apiKey })({ chain: mainnet }),
  // can pass an rpc provider directly
  [goerli.id]: new RpcProvider({ nodeUrl: 'http://my-node.com' }),
}

If you're ok with that maybe you can update the PR to reflect that design?

fracek commented 1 year ago

Actually, forget option 2. The providers argument should be renamed provider and accepts a ChainProviderFactory. If users want chain specific providers, we can provide a higher-level provider to combine two providers.

IvanLudvig commented 1 year ago

I agree that the current design is flawed and I implemented your idea. Can you take a look, please? @fracek

fracek commented 1 year ago

Looks good! Much cleaner now. Please fix the tests and I'll merge it.

IvanLudvig commented 1 year ago

I fixed the tests

fracek commented 1 year ago

Perfect, thank you! @IvanLudvig Are you on onlydust? How much time did you spend on this PR?

IvanLudvig commented 1 year ago

I just joined onlydust. I spent 3 hours