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

Bring over BX gas resources #5744

Closed
wants to merge 19 commits into from
Closed

Bring over BX gas resources #5744

wants to merge 19 commits into from

Conversation

walmat
Copy link
Contributor

@walmat walmat commented May 17, 2024

Fixes APP-####

What changed (plus any additional context for devs)

Screen recordings / screenshots

What to test

Copy link
Member

@benisgold benisgold left a comment

Choose a reason for hiding this comment

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

primarily just want to verify the necessity of caching/persisting these new queries. some other nits

src/__swaps__/screens/Swap/hooks/useSwapGas.ts Outdated Show resolved Hide resolved
createQueryKey(
'estimateApprovalGasLimitQueryKey',
{ chainId, ownerAddress, assetAddress, spenderAddress, assetType },
{ persisterVersion: 1 }
Copy link
Member

Choose a reason for hiding this comment

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

i'm curious about if/for how long we want to persist/cache this data. can gas limit change based on network congestion?

// Query Key

const estimateGasLimitQueryKey = ({ chainId, transactionRequest }: EstimateGasLimitArgs) =>
createQueryKey('estimateGasLimit', { chainId, transactionRequest }, { persisterVersion: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

same as above comment

// Query Key

const estimateSwapGasLimitQueryKey = ({ chainId, quote, assetToSell, assetToBuy }: EstimateSwapGasLimitArgs) =>
createQueryKey('estimateSwapGasLimit', { chainId, quote, assetToSell, assetToBuy }, { persisterVersion: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

same as above, esp bc quote will for sure change frequently

});

const getMeteorologyNetworkFromChainId = (chainId: ChainId) => {
switch (chainId) {
Copy link
Member

Choose a reason for hiding this comment

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

do we need blast and degen here also

// ///////////////////////////////////////////////
// Query Key

const meteorologyQueryKey = ({ chainId }: MeteorologyArgs) => createQueryKey('meteorology', { chainId }, { persisterVersion: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

same question about persistence

Copy link
Member

Choose a reason for hiding this comment

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

and caching

// Query Key

const optimismL1SecurityFeeQueryKey = ({ transactionRequest, chainId }: OptimismL1SecurityFeeArgs) =>
createQueryKey('optimismL1SecrityFee', { transactionRequest, chainId }, { persisterVersion: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

same q as above

also nit but the query key is misspelled haha optimismL1SecrityFee -> optimismL1SecurityFee

// ///////////////////////////////////////////////
// Query Key

const providerGasQueryKey = ({ chainId }: ProviderGasArgs) => createQueryKey('providerGas', { chainId }, { persisterVersion: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

same

@walmat walmat marked this pull request as draft May 21, 2024 14:27
@walmat
Copy link
Contributor Author

walmat commented May 21, 2024

will be splitting this up into separate PRs

@walmat walmat closed this May 21, 2024
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.

None yet

2 participants