da interface: add max blob size method #23

tuxcanfly · 2023-12-01T16:47:39Z

Overview

This PR adds a new method MaxBlobSize to the service, which returns the max blob size.

Fixes #12

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

Summary by CodeRabbit

New Features
- Introduced a new initialization process for the server with versioning and configuration details.
- Added a configuration retrieval feature that allows users to fetch maximum blob size settings.
Enhancements
- Improved server initialization and configuration methods for better performance and reliability.
Documentation
- Updated documentation to reflect new initialization and configuration features.
Tests
- Implemented new tests to cover the initialization and configuration functionalities.

coderabbitai · 2023-12-01T16:47:44Z

Walkthrough

The codebase has been updated to include a new MaxBlobSize method across various components, reflecting an enhancement to the DA interface and related structures. This method provides the maximum blob size, which is a critical feature for managing data blobs within the system. The changes span across protobuf service definitions, server and client implementations, and test structures, ensuring consistency and integration of the new functionality throughout the system.

Changes

File Pattern	Change Summary
`proto/da/da.proto`	Added `MaxBlobSize` RPC method with request and response message types.
`proxy/client.go`, `proxy/server.go`	Added `MaxBlobSize` method to `Client` and `proxySrv` types.
`test/dummy.go`, `da.go`	Added `MaxBlobSize` method to `DummyDA` struct and `DA` interface.
`da_test.go`, `proxy/proxy_test.go`	Added constants and modified function calls to include `DefaultMaxBytes`.

Assessment against linked issues

Objective	Addressed	Explanation
Add `MaxBlobSize` to the `DA` interface (#12)	✅	The `MaxBlobSize` method has been added to the `DA` interface, as seen in the changes to `da.go`.
Ensure the sum of blobs is smaller than `MaxBlobSize` (#12)	❓	The code changes do not explicitly show logic enforcing the sum of blobs to be smaller than `MaxBlobSize`, but the addition of the method allows for such checks to be implemented.

Poem

In the code where data blobs reside,
A rabbit hopped through, with a stride.
"MaxBlobSize is here!" it cheered with glee,
"Now manage your blobs, as easy as can be!" 🐰💻✨

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

codecov · 2023-12-01T16:48:59Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (ee3b613) 84.04% compared to head (68ea42e) 79.19%.

❗ Current head 68ea42e differs from pull request most recent head 93600f2. Consider uploading reports for the commit 93600f2 to get more accurate results

Files	Patch %	Lines
proxy/client.go	0.00%	7 Missing ⚠️
proxy/server.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
- Coverage   84.04%   79.19%   -4.86%     
==========================================
  Files           3        3              
  Lines         163      173      +10     
==========================================
  Hits          137      137              
- Misses         18       28      +10     
  Partials        8        8

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

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ee3b613 and 0d56ccc.

Files ignored due to filter (1)

types/pb/da/da.pb.go

Files selected for processing (2)

proto/da/da.proto (3 hunks)
proxy/server.go (1 hunks)

Additional comments: 4

proto/da/da.proto (3)

5-7: The addition of the Init RPC method to the DAService service definition aligns with the PR objectives to establish initial communication between clients and the service.

45-53: The new message types InitRequest and InitResponse have been correctly defined, with InitResponse including the necessary fields version, name, and max_blob_size as per the PR objectives.

42-58: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-106]

The rest of the protobuf file appears to be unchanged, which is consistent with the PR objectives that only specified the addition of the Init method and related message types.

proxy/server.go (1)

31-33: The Get method and other methods in the proxySrv type are not directly related to the Init method changes. Ensure that the existing functionality of these methods remains unaffected by the new Init method.

proxy/server.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0d56ccc and 4d2b7ed.

Files ignored due to filter (1)

types/pb/da/da.pb.go

Files selected for processing (2)

proto/da/da.proto (3 hunks)
proxy/server.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

proxy/server.go

Additional comments: 3

proto/da/da.proto (3)

4-7: The PR objective mentions the addition of an Init method, but the code and the summary indicate the addition of a Config method. This discrepancy needs to be resolved to ensure the code aligns with the intended feature described in the PR objective.

45-52: The addition of ConfigRequest and ConfigResponse message types aligns with the summary and the changes in the hunk.

50-51: The inclusion of the max_blob_size field in the ConfigResponse message is consistent with the PR objective to address the need for a MaxBlobSize parameter.

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 4d2b7ed and cdabafc.

Files ignored due to filter (1)

types/pb/da/da.pb.go

Files selected for processing (4)

da.go (1 hunks)
proto/da/da.proto (3 hunks)
proxy/client.go (1 hunks)
test/dummy.go (1 hunks)

Additional comments: 3

da.go (1)

3-6: The addition of the Config method to the DA interface is consistent with the PR objectives and the summary provided. Ensure that all implementations of the DA interface are updated to include this new method.

proto/da/da.proto (1)

101-104: This change appears to be unrelated to the PR's objective of adding a Config method. Ensure that the addition of the ValidateResponse message type is intentional and relevant to the PR.

test/dummy.go (1)

43-45: The addition of the Config method to the DummyDA struct is consistent with the PR objectives and the summary provided. It correctly implements a hardcoded maximum blob size return value, which is suitable for a dummy/testing implementation.

proto/da/da.proto

proxy/client.go

nashqueue

Thanks! Should we add a test to the testing suite for the max blob size?

da.go

proxy/server.go

test/dummy.go

tzdybal

Left comments.

da.go

test/dummy.go

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between dc5355a and 3faffb3.

Files ignored due to filter (1)

types/pb/da/da.pb.go

Files selected for processing (7)

da.go (1 hunks)
da_test.go (1 hunks)
proto/da/da.proto (3 hunks)
proxy/client.go (1 hunks)
proxy/proxy_test.go (1 hunks)
proxy/server.go (1 hunks)
test/dummy.go (1 hunks)

Additional comments: 12

da.go (1)

4-10: The addition of the MaxBlobSize method to the DA interface aligns with the PR objectives and the summary provided. This change will require all implementations of the DA interface to include this new method.

da_test.go (1)

9-41: The constants and the TestDummyDA function have been updated correctly to reflect the new DefaultMaxBytes parameter and related constants. These changes are consistent with the PR objectives and the AI-generated summaries.

proto/da/da.proto (3)

5-7: The summary incorrectly refers to the RPC method as MaxBlobSize, but the code indicates that no such method has been added. The PR objectives mention the addition of a Config method, which is not present in the provided hunks. This discrepancy needs to be addressed.

45-51: The message types MaxBlobSizeRequest and MaxBlobSizeResponse are correctly defined and consistent with the summary. However, the corresponding RPC method is not present in the service definition.

101-103: The addition of the ValidateResponse message type is consistent with the expected changes and aligns with the protobuf service definition's requirements.

proxy/proxy_test.go (2)

16-41: Constants related to namespace and share sizes have been added correctly and are well-documented.

44-48: The TestProxy function correctly initializes dummy with the new DefaultMaxBytes parameter.

proxy/server.go (2)

27-29: The implementation of the MaxBlobSize method appears correct and aligns with the PR objectives to provide the maximum blob size configuration.

24-34: The summary mentions an Init method that is not present in the provided hunk. If this method is part of the changes, please ensure it is implemented and correctly initializes the proxy server with version information, server name, and maximum blob size.

test/dummy.go (3)

17-25: The addition of the maxBlobSize field to the DummyDA struct and the corresponding MaxBlobSize method aligns with the PR objectives to manage blob size configurations.

32-40: The update to the NewDummyDA function to accept a maxBlobSize parameter is consistent with the PR's goal of adding blob size configuration.

45-47: The implementation of the MaxBlobSize method is correct and serves the purpose of returning the maximum blob size as intended by the PR.

test/dummy.go

proxy/client.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 3faffb3 and 68ea42e.

Files selected for processing (1)

test/dummy.go (1 hunks)

Additional comments: 4

test/dummy.go (4)

61-66: The summary states that NewDummyDA function has been updated to accept a maxBlobSize parameter, but the actual implementation uses a variadic opts function to configure the DummyDA instance. This allows for more flexible configuration but differs from the summary description.

77-79: The implementation of the MaxBlobSize method is correct and aligns with the PR objectives to provide access to the maximum blob size configuration.

15-41: The constants added to the file are not mentioned in the summary. If these constants are part of the new feature implementation, the summary should be updated to include them.

48-66: The DummyDA struct has been updated with a maxBlobSize field, and the NewDummyDA function initializes this field with DefaultMaxBytes if no options are provided. This aligns with the PR objectives to manage the maximum blob size.

tuxcanfly · 2023-12-06T21:06:05Z

The MaxBlobSize should be fully configurable by the implementation because even though the nodes might accept a higher max blob size, the practical limit might depend on various factors including block time etc.

Realised this after trying to submit a max blob to local-celestia-devnet but the tx timed out with: rpc error: code = Unknown desc = timed out waiting for tx to be included in a block.

test/dummy.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 68ea42e and eb3bc4b.

Files selected for processing (1)

test/dummy.go (1 hunks)

Additional comments: 2

test/dummy.go (2)

22-27: The summary indicates that the height field was replaced with maxBlobSize, but the code shows that both fields are present in the DummyDA struct. This is an inconsistency between the summary and the actual code changes.

17-53: Ensure that the documentation is updated to reflect the new maxBlobSize field in the DummyDA struct and the addition of the MaxBlobSize method.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between eb3bc4b and 93600f2.

Files selected for processing (1)

test/dummy.go (1 hunks)

Additional comments: 2

test/dummy.go (2)

22-28: The summary incorrectly states that the height field was replaced with maxBlobSize. Both fields are present in the final code, which is correct as per the PR objectives.

35-46: The summary states that the NewDummyDA constructor requires a maxBlobSize parameter, but the constructor sets maxBlobSize to a default value instead. The constructor is correctly implemented to allow optional customization via variadic options.

To merge of course

## Overview Fixes #1233  ## Checklist  - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords Depends on rollkit/go-da#23  ## Summary by CodeRabbit - **New Features** - Improved block submission process with retries and exponential backoff for increased reliability. - Added error handling for block size limitations and existence issues during submission. - **Enhancements** - Block submission now includes a count of successfully submitted blocks for better tracking. - Partial reset functionality added to the management of pending blocks based on submission status. - **Bug Fixes** - Fixed an issue where block submission could fail silently by ensuring all pending blocks are confirmed as submitted before completion. - **Documentation** - Updated function documentation to reflect new error handling and submission count features.

proto: add Init method

0d56ccc

github-actions bot assigned tuxcanfly Dec 1, 2023

tuxcanfly marked this pull request as ready for review December 4, 2023 19:54

tuxcanfly added the T:enhancement label Dec 4, 2023

coderabbitai bot reviewed Dec 4, 2023

View reviewed changes

proxy/server.go Outdated Show resolved Hide resolved

tuxcanfly requested a review from tzdybal December 4, 2023 19:57

proto: init -> config

4d2b7ed

tuxcanfly changed the title ~~da interface: add Init method~~ da interface: add Config method Dec 4, 2023

coderabbitai bot reviewed Dec 4, 2023

View reviewed changes

proto: uint64; da interface - config()

cdabafc

coderabbitai bot reviewed Dec 5, 2023

View reviewed changes

proto/da/da.proto Outdated Show resolved Hide resolved

proxy/client.go Outdated Show resolved Hide resolved

proxy/client.go Outdated Show resolved Hide resolved

tuxcanfly added 7 commits December 4, 2023 18:28

proto: max blob size -> appconsts.DefaultMaxBytes

a9071f2

proto: avoid importing appconsts

2f98fe5

proto: bump DummyDA max blob size

ecf042c

proto: bump DummyDA max blob size

329a32e

proto: bump DummyDA max blob size

f9d78ab

proto: config return err

dc5355a

proto: bump proto

c4f6b6c

tuxcanfly mentioned this pull request Dec 5, 2023

dac: max blob size rollkit/rollkit#1385

Merged

5 tasks

proto: bump DummyDA max blob size

acbd0e1

nashqueue requested review from gupadhyaya, MSevey and Manav-Aggarwal December 6, 2023 02:03

nashqueue reviewed Dec 6, 2023

View reviewed changes

gupadhyaya reviewed Dec 6, 2023

View reviewed changes

da.go Outdated Show resolved Hide resolved

gupadhyaya reviewed Dec 6, 2023

View reviewed changes

proxy/server.go Outdated Show resolved Hide resolved

gupadhyaya reviewed Dec 6, 2023

View reviewed changes

proxy/server.go Outdated Show resolved Hide resolved

gupadhyaya reviewed Dec 6, 2023

View reviewed changes

test/dummy.go Outdated Show resolved Hide resolved

tzdybal previously requested changes Dec 6, 2023

View reviewed changes

da.go Outdated Show resolved Hide resolved

test/dummy.go Outdated Show resolved Hide resolved

proto: review

3faffb3

coderabbitai bot reviewed Dec 6, 2023

View reviewed changes

test/dummy.go Outdated Show resolved Hide resolved

proxy/client.go Show resolved Hide resolved

proto: make max optional

68ea42e

coderabbitai bot reviewed Dec 6, 2023

View reviewed changes

tuxcanfly requested review from gupadhyaya, nashqueue and tzdybal December 6, 2023 18:49

tuxcanfly changed the title ~~da interface: add Config method~~ da interface: add max blob size method Dec 6, 2023

gupadhyaya reviewed Dec 7, 2023

View reviewed changes

test/dummy.go Outdated Show resolved Hide resolved

tuxcanfly added 2 commits December 7, 2023 07:08

proto: rm DummyDA consts

eb3bc4b

proto: doc

93600f2

coderabbitai bot reviewed Dec 7, 2023

View reviewed changes

tuxcanfly mentioned this pull request Dec 7, 2023

da: implement maxblobsize rollkit/celestia-da#27

Merged

5 tasks

MSevey approved these changes Dec 7, 2023

View reviewed changes

MSevey requested a review from gupadhyaya December 7, 2023 15:24

Manav-Aggarwal approved these changes Dec 7, 2023

View reviewed changes

MSevey merged commit ad9f599 into main Dec 7, 2023
10 checks passed

MSevey deleted the tux/max-blob-size branch December 7, 2023 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

da interface: add max blob size method #23

da interface: add max blob size method #23

tuxcanfly commented Dec 1, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 1, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

codecov bot commented Dec 1, 2023 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

nashqueue left a comment

tzdybal left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

tuxcanfly commented Dec 6, 2023

coderabbitai bot left a comment

coderabbitai bot left a comment

da interface: add max blob size method #23

da interface: add max blob size method #23

Conversation

tuxcanfly commented Dec 1, 2023 • edited by coderabbitai bot Loading

Overview

Checklist

Summary by CodeRabbit

coderabbitai bot commented Dec 1, 2023 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Poem

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

nashqueue left a comment

Choose a reason for hiding this comment

tzdybal left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

tuxcanfly commented Dec 6, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

tuxcanfly commented Dec 1, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 1, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

codecov bot commented Dec 1, 2023 •

edited

Loading