search

celestiaorg / celestia-node

Celestia Data Availability Nodes
Apache License 2.0
900 stars 879 forks source link

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

Closed renaynay closed 2 weeks ago

renaynay commented 3 weeks ago

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 commented 3 weeks ago

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 :warning:
nodebuilder/das/constructors.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

walldiss commented 3 weeks ago

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.