chore: revive max blob size decorator #3479
Conversation
WalkthroughWalkthroughThe changes involve the introduction of a new Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Out of diff range and nitpick comments (3)
x/blob/ante/blob_share_decorator.go (1)
5-5: Remove unused import.The import
v1 "github.com/celestiaorg/celestia-app/v2/pkg/appconsts/v1"is not used in the code. Consider removing it to keep the imports clean.x/blob/ante/max_total_blob_size_ante.go (1)
1-10: Remove unused import.The import
v1 "github.com/celestiaorg/celestia-app/v2/pkg/appconsts/v1"is not used in the code. Consider removing it to keep the imports clean.x/blob/ante/blob_share_decorator_test.go (1)
8-9: Unused imports should be removed.The imports for
v1andv2are used, but ensure thattmprotoandversionare necessary. If they are not used elsewhere in the code, consider removing them to keep the import list clean.Also applies to: 16-17
| { | ||
| name: "want no error if appVersion v1 and 8 MiB blob", | ||
| pfb: &blob.MsgPayForBlobs{ | ||
| BlobSizes: []uint32{8 * mebibyte}, | ||
| }, | ||
| appVersion: v1.Version, | ||
| }, |
There was a problem hiding this comment.
Add assertions for wantErr in v1 test cases.
The test case for appVersion v1 does not include an assertion for wantErr. Even if no error is expected, it is good practice to explicitly assert that wantErr is nil.
{
name: "want no error if appVersion v1 and 8 MiB blob",
pfb: &blob.MsgPayForBlobs{
BlobSizes: []uint32{8 * mebibyte},
},
appVersion: v1.Version,
+ wantErr: nil,
},Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| { | |
| name: "want no error if appVersion v1 and 8 MiB blob", | |
| pfb: &blob.MsgPayForBlobs{ | |
| BlobSizes: []uint32{8 * mebibyte}, | |
| }, | |
| appVersion: v1.Version, | |
| }, | |
| { | |
| name: "want no error if appVersion v1 and 8 MiB blob", | |
| pfb: &blob.MsgPayForBlobs{ | |
| BlobSizes: []uint32{8 * mebibyte}, | |
| }, | |
| appVersion: v1.Version, | |
| wantErr: nil, | |
| }, |
rootulp
requested review from
evan-forbes and
ninabarbakadze
and removed request for
ramin and
cmwaters
Co-authored-by: CHAMI Rachid <[email protected]>
rootulp
dismissed stale reviews from evan-forbes, ninabarbakadze, and rach-id
via
5dcbae9
celestia-bot
requested review from
a team,
staheri14 and
evan-forbes
and removed request for
a team
Closes #3468 by reviving the max blob size decorator and making both decorators version aware so that: