Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

da interface: add submit options #27

Merged
merged 5 commits into from
Jan 4, 2024
Merged

Conversation

tuxcanfly
Copy link
Contributor

@tuxcanfly tuxcanfly commented Dec 18, 2023

Overview

This PR adds SubmitOptions to the Submit method which include the fee/gas params.

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 new submission options for transactions, including customizable fees and gas limits.
  • Enhancements

    • Updated the submission process to allow specifying transaction fees and gas limits.
  • Documentation

    • Added documentation for the new SubmitOptions feature.
  • Tests

    • Enhanced test suites to incorporate the new SubmitOptions parameter in transaction submissions.

Copy link

coderabbitai bot commented Dec 18, 2023

Walkthrough

A new feature for submitting transactions with custom fee and gas settings has been introduced across various files in the codebase. The SubmitOptions message type and structure have been added to handle these settings, and functions related to submission now include this new parameter, enhancing flexibility and control over transaction submission.

Changes

File Path Change Summary
proto/da/da.proto Introduced SubmitOptions message type; Updated SubmitRequest to include options field.
.../da.go Added SubmitOptions struct and DefaultSubmitOptions function; Updated Submit method signature.
proxy/server.go Modified Submit method to accept options parameter.
proxy/client.go Updated Submit function to take an additional options parameter.
proxy/util.go Added optionsPB2DA function to convert protocol buffer SubmitOptions to da.SubmitOptions.
test/dummy.go Altered Submit method to include options parameter; Method now returns an error.
test/test_suite.go Added SubmitOptions type alias and DefaultSubmitOptions variable; Updated Submit calls to include default options.

🐇
Code hops along, new features in sight,
Submit with options, both day and night.
Fees and gas in their place, aligned just right,
The rabbit's work done, the code now takes flight. 🚀
🐇

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?

Share

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • You can tag CodeRabbit on specific lines of code or entire files in the PR by tagging @coderabbitai in a comment. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • You can tag @coderabbitai in a PR comment and ask questions about the PR and the codebase. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid.
    • @coderabbitai read the files in the src/scheduler package and generate README in the markdown format.

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.

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.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@RollkitBot RollkitBot requested a review from S1nus December 18, 2023 15:19
@tuxcanfly tuxcanfly changed the title tux/submit-options da interface: add submit options Dec 18, 2023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ad9f599 and 2cb9d1d.
Files ignored due to filter (1)
  • types/pb/da/da.pb.go
Files selected for processing (6)
  • da.go (3 hunks)
  • proto/da/da.proto (1 hunks)
  • proxy/client.go (1 hunks)
  • proxy/server.go (1 hunks)
  • test/dummy.go (1 hunks)
  • test/test_suite.go (3 hunks)
Files skipped from review due to trivial changes (2)
  • da.go
  • proto/da/da.proto
Files skipped from review as they are similar to previous changes (3)
  • proxy/client.go
  • proxy/server.go
  • test/dummy.go
Additional comments: 3
test/test_suite.go (3)
  • 49-59: The Submit method calls have been correctly updated to include DefaultSubmitOptions as per the new feature implementation. Ensure that all tests are updated to reflect this change and that the error handling is consistent with the new method signature.

  • 119-119: The Submit method call within GetIDsTest function is correctly using the new DefaultSubmitOptions. This change is in line with the PR's objectives and ensures that the test suite remains consistent with the updated codebase.

  • 171-171: The Submit method call within the ConcurrentReadWriteTest function is correctly using the new DefaultSubmitOptions. This ensures that the test function is compatible with the updated Submit method signature.

test/test_suite.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

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

Comparison is base (a353302) 79.19% compared to head (d755bc9) 75.51%.

Files Patch % Lines
proxy/util.go 56.25% 6 Missing and 1 partial ⚠️
da.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
- Coverage   79.19%   75.51%   -3.69%     
==========================================
  Files           3        4       +1     
  Lines         173      196      +23     
==========================================
+ Hits          137      148      +11     
- Misses         28       39      +11     
- Partials        8        9       +1     

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2cb9d1d and e8a3026.
Files ignored due to filter (1)
  • types/pb/da/da.pb.go
Files selected for processing (3)
  • proto/da/da.proto (1 hunks)
  • proxy/server.go (1 hunks)
  • proxy/util.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • proto/da/da.proto
  • proxy/server.go
Additional comments: 1
proxy/util.go (1)
  • 24-32: The implementation of optionsPB2DA correctly checks for nil and uses da.DefaultSubmitOptions() if the input is nil. This is a good practice for handling optional parameters.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between e8a3026 and 7954d38.
Files selected for processing (1)
  • test/test_suite.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • test/test_suite.go

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 7954d38 and ae60407.
Files selected for processing (4)
  • da.go (3 hunks)
  • proxy/client.go (1 hunks)
  • proxy/util.go (1 hunks)
  • test/test_suite.go (3 hunks)
Files skipped from review due to trivial changes (1)
  • test/test_suite.go
Files skipped from review as they are similar to previous changes (2)
  • da.go
  • proxy/client.go

Copy link
Member

@Manav-Aggarwal Manav-Aggarwal left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! We should test out Submit with options in the test suite, can also be done in a follow up issue if you'd like

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a353302 and d755bc9.
Files ignored due to filter (1)
  • types/pb/da/da.pb.go
Files selected for processing (7)
  • da.go (3 hunks)
  • proto/da/da.proto (1 hunks)
  • proxy/client.go (1 hunks)
  • proxy/server.go (1 hunks)
  • proxy/util.go (1 hunks)
  • test/dummy.go (1 hunks)
  • test/test_suite.go (3 hunks)
Files skipped from review due to trivial changes (1)
  • test/test_suite.go
Additional comments: 6
da.go (1)
  • 3-8: The addition of the SubmitOptions struct with Fee and Gas fields is a good encapsulation of transaction parameters.
proxy/server.go (1)
  • 57-64: The modification of the Submit method to accept and pass options to target.Submit is approved. Ensure consistency in handling nil options across the codebase.
proxy/client.go (1)
  • 92-98: The modification of the Submit function to accept an additional options parameter is approved. Ensure consistency in handling nil options across the codebase.
proto/da/da.proto (2)
  • 84-88: The addition of the SubmitOptions message with fee and gas fields is approved.

  • 93-93: The modification to include an options field of type SubmitOptions in the SubmitRequest message is approved.

test/dummy.go (1)
  • 102-105: The modification of the Submit method to accept an additional options parameter is approved. Ensure consistency in handling nil options across the codebase.

proxy/util.go Show resolved Hide resolved
proxy/util.go Show resolved Hide resolved
da.go Show resolved Hide resolved
da.go Show resolved Hide resolved
@Manav-Aggarwal Manav-Aggarwal merged commit 7658c38 into main Jan 4, 2024
10 of 12 checks passed
@Manav-Aggarwal Manav-Aggarwal deleted the tux/submit-options branch January 4, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants