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

✨ feat: Add gas estimation actions #67

Merged

Conversation

roninjin10
Copy link
Contributor

@roninjin10 roninjin10 commented Sep 12, 2023

@vercel
Copy link

vercel bot commented Sep 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
op-viem-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 6, 2023 1:20am

Comment on lines +25 to +31
/**
* Estimate the l1 gas price portion for a transaction
* @example
* const price = await getL1GasPrice(publicClient, {
* blockNumber,
* blockTag,
* });
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
/**
* Estimate the l1 gas price portion for a transaction
* @example
* const price = await getL1GasPrice(publicClient, {
* blockNumber,
* blockTag,
* });
*/

Maybe delete for now and go through and add jsdocs in it's own pr

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I've been underinvesting in jsdoc in favor of the markdown docs, not sure if right tradeoff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jsdoc is more important because you see jsdoc in your editor imo

package.json Outdated Show resolved Hide resolved
// no way to get historical gas price
createPublicClient: (...args: [any]) => {
const client = _viem.createPublicClient(...args)
client.getGasPrice = async () => parseGwei('0.00000042')
Copy link
Collaborator

Choose a reason for hiding this comment

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

does getGasPrice not work with normal forking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not unfortunately. Viem types correctly don't give you blockTag as a param as well

* const contract = getGasPriceOracleContract(publicClient)
* const gasPrice = await contract.read.gasPrice({ blockNumber })
*/
const getGasPriceOracleContract = <TChain extends Chain | undefined>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

again prefer 1 function per file.

Thinking if we should actually export these viem versions of all the main contracts 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eth-optimism/contracts-ts is the package that exports versions of all the main contracts. It does it both as react hooks and as vanilla hooks. It might be a good idea to reexport them from this package though

Copy link
Contributor Author

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

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

Thanks for review! Will update in next couple days

package.json Outdated Show resolved Hide resolved
// no way to get historical gas price
createPublicClient: (...args: [any]) => {
const client = _viem.createPublicClient(...args)
client.getGasPrice = async () => parseGwei('0.00000042')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not unfortunately. Viem types correctly don't give you blockTag as a param as well

src/actions/public/L2/feeActions.spec.ts Outdated Show resolved Hide resolved
* const contract = getGasPriceOracleContract(publicClient)
* const gasPrice = await contract.read.gasPrice({ blockNumber })
*/
const getGasPriceOracleContract = <TChain extends Chain | undefined>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eth-optimism/contracts-ts is the package that exports versions of all the main contracts. It does it both as react hooks and as vanilla hooks. It might be a good idea to reexport them from this package though

src/actions/public/L2/feeActions.ts Outdated Show resolved Hide resolved
Comment on lines +25 to +31
/**
* Estimate the l1 gas price portion for a transaction
* @example
* const price = await getL1GasPrice(publicClient, {
* blockNumber,
* blockTag,
* });
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jsdoc is more important because you see jsdoc in your editor imo

src/actions/public/L2/feeActions.spec.ts Outdated Show resolved Hide resolved
src/actions/public/L2/feeActions.spec.ts Outdated Show resolved Hide resolved
src/actions/public/L2/feeActions.spec.ts Outdated Show resolved Hide resolved
src/decorators/publicL2OpStackActions.ts Show resolved Hide resolved
* args: [address],
* });
*/
export const estimateFees: EstimateFees = async (client, options) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in an ideal world, this would be the default estimateGas function for an OPStack chain in viem, right? @roninjin10

Do we think we should name it like opStackEstimateGas or something closer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Estimate gas is different. On l1 estimate fees would be estimateGas multiplied by the gas price. This is something I chatted with Jake about and he lightly agreed it would be valuable to add for all chains

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok thanks for clarifying @roninjin10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not in love with the name estimateFees btw I just couldn’t come up with a better name. Etherscan calls it a transactionFee. Maybe we should call it transactionFee? I would love a name that makes it so folks can guess the difference between it and estimateGas most of the time

Copy link
Collaborator

Choose a reason for hiding this comment

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

transactionFee is pretty good! I don't have a strong preference

Copy link
Collaborator

@wilsoncusack wilsoncusack left a comment

Choose a reason for hiding this comment

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

@roninjin10 we should also export this in actions/index

@roninjin10 roninjin10 temporarily deployed to verify October 3, 2023 14:55 — with GitHub Actions Inactive
@roninjin10 roninjin10 force-pushed the 09-12-feat_Add_gas_estimation_actions branch from 96d4f79 to ca4bb91 Compare October 3, 2023 14:55
@roninjin10 roninjin10 temporarily deployed to verify October 3, 2023 14:55 — with GitHub Actions Inactive
@wbnns wbnns added the type: enhancement New feature or request label Oct 4, 2023
src/actions/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@zencephalon zencephalon left a comment

Choose a reason for hiding this comment

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

Approving with fast-follows for documentation and testing updates.

@zencephalon zencephalon merged commit 2f5a5ac into base-org:main Oct 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants