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

refactor(blob)!: replace SubmitOptions with a GasPrice parameter #3094

Merged
merged 6 commits into from
Jan 23, 2024

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Jan 11, 2024

This PR modifies the blob interface to replace SubmitOptions with GasLimit. The reasoning here is that setting a gas price is a lot more intuitive/easier than calculating the fee and gas limit, which take cosmossdk and celestia-app imports to calculate without much effort. Related: rollkit/go-da#30

Based on a suggestion from @vgonkivs we alias float64 to be able to provide a default value, but now that its there I think we can just have float64 and the same constructor, as it doesn't change that much.

Breaks the API, cc @jcstein, @tuxcanfly, and @Ferret-san .

Closes #3053.

Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

What has happened with the order of gomock import in /mocks/?

blob/service.go Outdated Show resolved Hide resolved
@distractedm1nd
Copy link
Collaborator Author

What has happened with the order of gomock import in /mocks/?

It looks like this is now correct, but I'm also not sure

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (7bf9713) 51.96% compared to head (92211be) 51.85%.
Report is 1 commits behind head on main.

Files Patch % Lines
blob/service.go 0.00% 11 Missing ⚠️
libs/pidstore/pidstore.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3094      +/-   ##
==========================================
- Coverage   51.96%   51.85%   -0.12%     
==========================================
  Files         178      178              
  Lines       11206    11226      +20     
==========================================
- Hits         5823     5821       -2     
- Misses       4888     4906      +18     
- Partials      495      499       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

renaynay
renaynay previously approved these changes Jan 22, 2024
blob/service.go 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.

I agree with the goal of making this path easier to use. IMO a user shouldn't have to explicitly specify their gas price, gas limit, or fee.

  1. If a user doesn't specify their gas price, the default gas price of .002 should be used.
  2. If a user doesn't specify their gas limit, one is estimated based on the transaction contents (i.e. blob sizes)
  3. If a user doesn't specify their fee, it is calculated based on fee = gasPrice * gasLimit.

nodebuilder/blob/cmd/blob.go Outdated Show resolved Hide resolved
nodebuilder/blob/cmd/blob.go Outdated Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

also utack

@distractedm1nd distractedm1nd enabled auto-merge (squash) January 23, 2024 22:52
@distractedm1nd distractedm1nd merged commit debad37 into main Jan 23, 2024
27 checks passed
@distractedm1nd distractedm1nd deleted the gas-price branch January 23, 2024 23:06
@musalbas
Copy link
Member

If we're removing the ability to set gas limit, we need to be 100% the gas limit estimate is correct, as I have heard reports from teams that the estimated gas limit is lower than it actually should be. This is why wallets like Keplr multiply gas limit estimates 1.2x

@jcstein jcstein mentioned this pull request Jan 24, 2024
5 tasks
@jcstein
Copy link
Member

jcstein commented Feb 7, 2024

has this been tested?

@jcstein
Copy link
Member

jcstein commented Feb 8, 2024

I just did and it worked for me

celestia version
export NODE_STORE=$HOME/.celestia-light-mocha-4
celestia blob submit 0x6B656B 'kek' --gas.price 0.0069 --node.store $NODE_STORE
Semantic version: v0.13.0
Commit: e55e1c88708b46839867bcbbed9bcdd8a3ffa830
Build Date: Wed Feb  7 19:32:07 EST 2024
System version: arm64/darwin
Golang version: go1.21.1
{
  "result": {
    "height": 1126617,
    "commitment": "cjtmIuILvB4ItexsJoHux/G6Yp15w6YmpSbXkSlS2ZE="
  }
}

celenium link to tx

@jcstein
Copy link
Member

jcstein commented Feb 8, 2024

Screenshot 2024-02-08 at 10 57 11 AM however, i'm not sure if this is accurate?

@jcstein
Copy link
Member

jcstein commented Feb 9, 2024

How does this apply to the remainder of methods? context in this issue #3174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blob kind:break! Attached to breaking PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Allow clients to configure Gas Price in SubmitOptions
9 participants