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

improvement!(blob/service): add options to submit #2630

Merged
merged 7 commits into from
Sep 7, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Aug 29, 2023

Overview

Allows to configure fee and gasPrice in blob.Submit.
Closes #2629

Will update blob's rpc in a follow-up PR.

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

FYI @jcstein , @nashqueue

@vgonkivs vgonkivs self-assigned this Aug 29, 2023
@vgonkivs vgonkivs added kind:misc Attached to miscellaneous PRs kind:break! Attached to breaking PRs area:blob labels Aug 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Merging #2630 (c83e6f8) into main (196e849) will decrease coverage by 0.19%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2630      +/-   ##
==========================================
- Coverage   51.05%   50.87%   -0.19%     
==========================================
  Files         158      159       +1     
  Lines       10551    10614      +63     
==========================================
+ Hits         5387     5400      +13     
- Misses       4690     4736      +46     
- Partials      474      478       +4     
Files Changed Coverage Δ
blob/batch.go 0.00% <0.00%> (ø)
blob/service.go 71.60% <0.00%> (-1.24%) ⬇️
cmd/celestia/blob.go 9.86% <0.00%> (-0.07%) ⬇️

... and 12 files with indirect coverage changes

blob/options.go Outdated Show resolved Hide resolved
blob/options.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/options.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

After a discussion with @distractedm1nd, we figured out that the Option param won't work on the RPC because 'Option' is a typedef on a func() and marshalling functions/methods does not make much sense, so the request will fail. We agreed to have one more endpoint SubmitWithFee that will have fee+gaslimit as input params, and Submit will be used as blobs submitter with the default fee and gasLim

@vgonkivs
Copy link
Member Author

vgonkivs commented Aug 29, 2023

One more idea came to my mind: we can rework blob.Submit by getting new type blob.Batch.

type Batch struct {
   Blob []*Blob
   Fee int64
  GasLimit uint64
}

func NewBatch(blobs *[]Blob) *Batch {
  return &Batch {
     Blob: blobs,
     Fee: -1,
}

type func(*Batch) Option

func WithFee(int64) Option {}
func WithGasLimit(uint64) Option {}
}

func(s *service)Submit(ctx context.Contex, blobs *Batch){}

@vgonkivs
Copy link
Member Author

Anyway we are going to make breaking changes but the second approach looks better to me as we will still have a single endpoint for the Submit

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.

@vgonkivs please don't break the method yet - let's just extend it by adding the additional SubmitWithOptions and have a struct called SubmissionOptions or just Options that contains fee and gas limit as field

i think the concept of batch is unnecessarily confusing

@vgonkivs
Copy link
Member Author

ok

ramin
ramin previously approved these changes Aug 30, 2023
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

i like this and changes are good. I'll defer to @renaynay and @Wondertan on if they approve/deny the breaking change to Submit RPC but the things i suggested == :shipit:

@vgonkivs vgonkivs requested a review from renaynay August 30, 2023 14:12
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.

Fine w me

nodebuilder/node/admin.go 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.

Please make an issue to fix variadics and gtg

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 kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blob: add WithFee and WithGasLimit options to Submit
7 participants