Skip to content
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

Define the basis for a generic and customizable BalancerSdkConfig #1

Open
wants to merge 1 commit into
base: daniel/subgraph-module
Choose a base branch
from

Conversation

danielmkm
Copy link

@danielmkm danielmkm commented Dec 27, 2021

This PR builds upon balancer#20

The BalancerSdkConfig should serve as the basis for a global config object with the goal of providing both a simple way to create the SDK for standard usecases and the ability to customize and extend the SDK when necessary.

export interface BalancerSdkConfig {
    //use a known network or provide an entirely custom config
    network: Network | BalancerNetworkConfig;
    rpcUrl: string;
    //default to JsonRpcProvider if not provided
    provider?: Provider;
    //overwrite the subgraph url if you don't want to use the balancer labs maintained version
    customSubgraphUrl?: string;
    //optionally overwrite parts of the standard SOR config
    sor?: Partial<BalancerSdkSorConfig>;
}

For the standard usecase, you'll be able to create the SDK with only the network ID and an rpc url:

const balancerSdk = new BalancerSdk({ network, rpcUrl});

We leverage the flexibility of typescript types to allow for the definition of the network to be simply the chain id OR a fully defined network config.

export interface BalancerSdkConfig {
    ...
    //use a known network or provide an entirely custom config
    network: Network | BalancerNetworkConfig;
    ...
}

export enum Network {
    MAINNET = 1,
    ROPSTEN = 3,
    RINKEBY = 4,
    GÖRLI = 5,
    KOVAN = 42,
    POLYGON = 137,
    ARBITRUM = 42161
}

export interface BalancerNetworkConfig {
    chainId: Network;
    vault: string;
    weth: string;
    multicall: string;
    staBal3Pool?: {
        id: string;
        address: string;
    };
    wethStaBal3?: {
        id: string;
        address: string;
    };
    subgraphUrl: string;
}

We provide the option to selectively overwrite data sources for the SOR. This should extend out into other parts of the SDK where applicable. Default config that is customisable where it makes sense.

export interface BalancerSdkSorConfig {
    //use a built-in service or provide a custom implementation of a TokenPriceService
    //defaults to coingecko
    tokenPriceService: 'coingecko' | 'subgraph' | TokenPriceService;
    //use a built-in service or provide a custom implementation of a PoolDataService
    //defaults to subgraph
    poolDataService: 'subgraph' | PoolDataService;
    //if a custom PoolDataService is provided, on chain balance fetching needs to be handled externally
    //default to true.
    fetchOnChainBalances: boolean;
}

Additionally in this PR:

  • Use the refactored SOR package, this will need to get transitioned over to balancer-labs once its available.
  • Define per network configs for all known networks

network: Network | BalancerNetworkConfig;
rpcUrl: string;
//default to JsonRpcProvider if not provided
provider?: Provider;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid accepting ethers types as inputs as much as possible. There can often be TS errors thrown because of mismatching ethers versions in these cases, especially around providers. Since we are accepting a rpcUrl then the JsonRpcProvider should always be initialized within the SDK. The only thing we might need to accept as an input here might be a Signer. wdyt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally fine for me. We default require the rpcUrl and initialize the JsonRpcProvider within the SDK.

As for the signer I'd say we wait until we have a need for it. At the moment I'd say we leave any signing external to the SDK.

Copy link
Author

@danielmkm danielmkm Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @garethfuller, I went ahead and merged these PRs into a single on pointing at the balancer labs repo balancer#22.

The change for the above is in this comment: balancer@3a10856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants