-
Notifications
You must be signed in to change notification settings - Fork 3
feat/add_gas_heuristics_details #176
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
Conversation
WalkthroughThe changes update the documentation by improving the formatting of markdown tables in the "Reduce-Only Order Precedence" section for consistency and readability. Additionally, a new "Gas estimation" section is introduced at the end of the document, providing an overview of gas consumption on the Injective Chain, methods for estimating gas, and upcoming deterministic gas requirements for certain message types. The new section includes explanatory text and a detailed table outlining fixed gas units for specific message types. No changes to code logic or control flow are present; all updates are informational. Changes
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the documentation for the Exchange module by adding details about fixed gas requirements. The changes include updating table header formatting in the order information sections and adding a new "Gas estimation" section with a detailed table of fixed gas values for various message types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
source/includes/_overview.md (3)
244-244
: Add comma before “and” for clarity.Use a comma to separate the two independent clauses in this sentence:
- This is perfectly valid and no further action is required. + This is perfectly valid, and no further action is required.🧰 Tools
🪛 LanguageTool
[uncategorized] ~244-~244: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...Reduce-only | This is perfectly valid and no further action is required. If the b...(COMMA_COMPOUND_SENTENCE_2)
273-273
: Add missing comma after introductory phrase.For better readability, insert a comma after “Again” and before “and”:
- Again this perfectly valid and no further action is required because... + Again, this is perfectly valid, and no further action is required because...🧰 Tools
🪛 LanguageTool
[uncategorized] ~273-~273: A comma might be missing here.
Context: ...63,500 | 0.1 BTC | Reduce-only | Again this perfectly valid and no further act...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
349-355
: Consider adding a blank line before the batch-messages list.A blank line between the explanatory sentence and the bullet list improves markdown rendering and readability:
- This applies similarly to the following batch messages: - - `MsgBatchCreateSpotLimitOrders` + This applies similarly to the following batch messages: + + - `MsgBatchCreateSpotLimitOrders` + - `MsgBatchCancelSpotOrders` + - `MsgBatchCreateDerivativeLimitOrders` + - `MsgBatchCancelDerivativeOrders` + - `MsgBatchCancelBinaryOptionsOrders`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/includes/_overview.md
(3 hunks)
🧰 Additional context used
🪛 LanguageTool
source/includes/_overview.md
[uncategorized] ~244-~244: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...Reduce-only | This is perfectly valid and no further action is required. If the b...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~273-~273: A comma might be missing here.
Context: ...63,500 | 0.1 BTC | Reduce-only | Again this perfectly valid and no further act...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 markdownlint-cli2 (0.17.2)
source/includes/_overview.md
320-320: Link fragments should be valid
null
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - injective-docs
- GitHub Check: Header rules - injective-docs
- GitHub Check: Pages changed - injective-docs
- GitHub Check: test (3.2)
🔇 Additional comments (9)
source/includes/_overview.md (9)
235-242
: Approve table formatting improvements in reduce-only order example.The updated alignment and spacing enhance readability and consistency of the markdown table for the reduce-only order illustration.
246-253
: Approve table formatting improvements in second reduce-only example.The consistent table style makes the example for a 0.5 BTC reduce-only order clearer.
265-272
: Approve table formatting improvements in vanilla limit order example.The revised alignment and bold styling improve clarity for the vanilla limit order case.
314-314
: New “Gas estimation” section introduced.This heading appropriately signals the start of the gas heuristics documentation.
316-321
: Clear summary of gas estimation methods.The two approaches—transaction simulation and historical estimation via the SDK—are described succinctly and are easy for users to follow.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
320-320: Link fragments should be valid
null(MD051, link-fragments)
322-323
: Clarify the use of fixed gas values as approximations.The note conveys that these fixed values can already serve as a reliable approximation prior to full implementation, which helps users plan their transactions.
324-324
: Heading “Fixed gas requirement” is clear.This subsection title accurately describes the content that follows.
326-327
: Introductory text for fixed gas table is informative.It properly sets expectations by noting that unlisted messages will continue to have non-deterministic gas.
328-347
: Fixed gas table is comprehensive and well-formatted.All message types and their gas units are clearly presented in a consistent layout.
There are two primary methods for determining the gas required for a transaction: | ||
|
||
- Transaction simulation: The chain allows users to submit transactions in simulation mode. In this mode, the transaction is executed as if it were part of the next chain block, and a response is returned to the user. This response includes the gas required for the simulation. It is advisable to slightly increase the simulated gas when broadcasting the actual transaction to minimize the risk of encountering an out-of-gas error. | ||
- Gas estimation: Users can estimate the gas required for a transaction based on the messages included and the historical gas consumption of similar messages processed on-chain. For users utilizing the Injective Python SDK, the [message broadcaster](#message-broadcaster) can assist in this estimation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the “#message-broadcaster” link fragment.
The anchor [message broadcaster](#message-broadcaster)
may not correspond to an existing header in this document. Please ensure a matching ### Message broadcaster
section is present, or update the link target to an appropriate heading.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
320-320: Link fragments should be valid
null
(MD051, link-fragments)
Summary by CodeRabbit