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

chore(nodebuilder/das | pruner): Privatise samplingWindow in DAS params, add Duration method to AvailabilityWindow #3378

Merged
merged 5 commits into from
May 15, 2024

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented May 8, 2024

While this isn't really a bug fix, it's more of a "UX" fix - I'm removing SamplingWindow from the node's config so that it doesn't confuse users to see "0ms" as the config value even though in practice it's 30 days.

The reason there's a discrepancy is because we force the light sampling window in the DASer for light nodes in nodebuilder anyway so that config value is ignored regardless, meaning it does not need to be exposed.

Also adding Duration() method to return time.Duration instead of having to cast everywhere.

@codecov-commenter
Copy link

codecov-commenter commented May 8, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 44.73%. Comparing base (2469e7a) to head (088901f).
Report is 66 commits behind head on main.

Files Patch % Lines
pruner/window.go 50.00% 2 Missing ⚠️
nodebuilder/das/constructors.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3378      +/-   ##
==========================================
- Coverage   44.83%   44.73%   -0.10%     
==========================================
  Files         265      272       +7     
  Lines       14620    15271     +651     
==========================================
+ Hits         6555     6832     +277     
- Misses       7313     7657     +344     
- Partials      752      782      +30     

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

pruner/window.go Show resolved Hide resolved
nodebuilder/das/config.go Outdated Show resolved Hide resolved
ramin
ramin previously approved these changes May 8, 2024
pruner/find.go Show resolved Hide resolved
nodebuilder/das/config.go Outdated Show resolved Hide resolved
@walldiss
Copy link
Member

walldiss commented May 8, 2024

I have a couple of suggestions that could simplify the implementation:

  1. Could we incorporate the default value for the SamplingWindow directly within the [DefaultParameters()](https://github.com/celestiaorg/celestia-node/blob/c04bbd251e7947180aadba793255e16e3126ef56/das/options.go#L56-L63) function? Could be just one-line addition.

  2. Given that the SamplingWindow value is always overwritten in our current setup, exposing this setting to users seems unnecessary and potentially misleading. We should consider making SamplingWindow config field non-public. This approach would help prevent any confusion and ensure our configuration remains streamlined and intuitive.

@renaynay renaynay changed the title chore(nodebuilder/das | pruner): Add default value for SamplingWindow to DAS config, add Duration method to AvailabilityWindow chore(nodebuilder/das | pruner): Privatise samplingWindow in DAS params, add Duration method to AvailabilityWindow May 13, 2024
das/options.go Outdated Show resolved Hide resolved
@renaynay renaynay enabled auto-merge (squash) May 13, 2024 13:59
@renaynay renaynay merged commit 007dae7 into celestiaorg:main May 15, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants