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

CIP-18: Standardised Gas and Pricing Estimation Interface #100

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

cmwaters
Copy link
Contributor

Overview

I have submitted this as part of the Interface category, but it may be better suited as a CRC. Let me know and I can easily change it

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Really well written

cips/cip-tx-interface.md Outdated Show resolved Hide resolved

The effectiveness of a "standardised" interface depends on the willingness of current and future clients to adopt it as well as the willingness of teams to provide those services. To set a sufficient precendent, both the Node API within `celestia-node` and the consensus node within `celestia-app` will implement client and server implementations respectively, creating an interface between the existing line of communication. That way by default, light nodes will use that API with the trusted provider they are already using for transaction submission.

The consensus node will use the SimulateTx method to estimate the gas used and use the `min_gas_price` parameter within state as the `estimated_gas_price`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think SimulateTx is often used in conjunction with

      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)

so should the initial implementation return the direct output of SimulateTx or multiplied by some factor (i.e. 1.1 or 1.2)?

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 don't have strong feelings on this. I would lean towards multiplying by 1.1

title: Standardised Gas and Pricing Estimation Interface
description: A standardised interface for estimating gas usage and gas pricing for transactions
author: Callum Waters (@cmwaters)
discussions-to: URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: this needs a URL pre-merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I haven't yet created the forum post but will do that today

cips/cip-tx-interface.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

CIP content LGTM!

I think the CIP editors need to do the following before it can be merged:

  1. assign a CIP number
  2. rename the file
  3. add it to the table of contents in the README

@jcstein jcstein changed the title CIP: Standardised Gas and Pricing Estimation Interface CIP-18: Standardised Gas and Pricing Estimation Interface Mar 19, 2024
@jcstein jcstein merged commit d8fb5da into celestiaorg:main Mar 19, 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.

3 participants