Skip to content

Conversation

hopeyen
Copy link
Contributor

@hopeyen hopeyen commented Feb 4, 2025

Why are these changes needed?

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 :(

@@ -29,6 +29,8 @@ message DisperseBlobRequest {
// The blob to be dispersed.
//
// The size of this byte array may be any size as long as it does not exceed the maximum length of 16MiB.
// While there is no minimum size, the blob size charged against the payment method will be rounded up to
// the nearest multiple of `minNumSymbols` defined by the payment vault contract (https://github.com/Layr-Labs/eigenda/blob/1430d56258b4e814b388e497320fd76354bfb478/contracts/src/payments/PaymentVaultStorage.sol#L9).
// (In the future, the 16MiB limit may be increased, but this is not guaranteed to happen.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// (In the future, the 16MiB limit may be increased, but this is not guaranteed to happen.)
is this not true anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the line but still curious

Copy link
Contributor

Choose a reason for hiding this comment

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

Just not quite useful to mention a "maybe" future that doesn't have any specifics at the API.

When we actually increase it, we'll announce it loud and clear anyway (and update this documentation).

@@ -29,6 +29,8 @@ message DisperseBlobRequest {
// The blob to be dispersed.
//
// The size of this byte array may be any size as long as it does not exceed the maximum length of 16MiB.
// While there is no minimum size, the blob size charged against the payment method will be rounded up to
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be 0? len(blob)>0 is required right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, fixed the sentence

@hopeyen hopeyen merged commit 37a4d6d into master Feb 4, 2025
9 checks passed
@hopeyen hopeyen deleted the hope/pmt-min-symbols-api-doc branch February 4, 2025 21:26
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.

2 participants