Skip to content

Conversation

hopeyen
Copy link
Contributor

@hopeyen hopeyen commented Feb 25, 2025

Why are these changes needed?

https://linear.app/eigenlabs/issue/EGDA-996/eda2-10-[medium]-avoid-lossy-and-unnecessary-casting

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@hopeyen hopeyen force-pushed the hope/misc-casting branch 5 times, most recently from dbcfd74 to ea4dd18 Compare February 26, 2025 01:08
@hopeyen hopeyen marked this pull request as ready for review February 26, 2025 02:08
@hopeyen hopeyen requested review from ian-shim and anupsv February 26, 2025 02:08
currentReservationPeriod := meterer.GetReservationPeriodByNanosecond(timestamp, a.reservationWindow)
symbolUsage := uint64(a.SymbolsCharged(numSymbols))
symbolUsage := a.SymbolsCharged(numSymbols)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the a max value for numSymbols that we should be checking ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not enforced by payments, I think it will be checked in validation before it gets to meterer. If you think it is within the meterer scope and should double check, I can add config to the meterer for a maximum number of blobs and add a check

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's enforced somewhere else and it's the only call path then no need.

@hopeyen hopeyen merged commit d189f77 into master Feb 28, 2025
12 checks passed
@hopeyen hopeyen deleted the hope/misc-casting branch February 28, 2025 04:37
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