-
Notifications
You must be signed in to change notification settings - Fork 2
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
[MEVBlocker] cleanup after fee increase to 30% #133
Conversation
WalkthroughThis pull request updates three SQL queries across different modules. In the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 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.
Actionable comments posted: 0
🧹 Nitpick comments (8)
mevblocker/fees/value_per_tx_4188777.sql (8)
16-17
: Correct Spelling in Comment
The comment currently reads “perfomance optimisation” – please fix the typo to “performance optimisation” for clarity.
17-25
: Union Clause and CAST Keyword Consistency
Within the nonexclusive_flow CTE, the UNION operator is used without a qualifier. To meet style guidelines, explicitly use “UNION DISTINCT” (since duplicates are being removed). Additionally, update the expression from
cast(hash as varchar)
to
CAST(hash AS varchar)
to ensure consistent casing of SQL keywords.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 23-23: 'UNION [DISTINCT|ALL]' is preferred over just 'UNION'.
[warning] 24-24: Function names must be consistently upper case.
[warning] 24-24: Keywords must be consistently upper case.
147-147
: Updated Filter in user_txs CTE
The added condition now excludes transactions found in nonexclusive_flow. Please update “CAST(tx.hash AS VARCHAR)” to “CAST(tx.hash AS varchar)” so that the datatype is lowercased per the style guidelines.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 147-147: Datatypes must be consistently lower case.
61-62
: Consistent Data Type Casing in CAST Function
In the mev_blocker_tx CTE, the expressions
CAST(JSON_EXTRACT(mb.transactions, '$[0].hash') AS VARCHAR)
and
CAST(JSON_EXTRACT(mb.transactions, '$[0].from') AS VARCHAR)
should be updated to use “varchar” (i.e. AS varchar) to satisfy the requirement that datatypes be lowercase.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 61-61: Datatypes must be consistently lower case.
[warning] 62-62: Datatypes must be consistently lower case.
70-71
: Standardize Datatype Casing in JSON_TABLE
In the JSON_TABLE definition, change “VARCHAR(255)” to “varchar(255)” so that datatypes are consistently in lower case throughout the query.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 71-71: Datatypes must be consistently lower case.
120-120
: Lowercase Data Type in CAST Expression (kickback_txs)
Please modify “CAST(et.gas_used AS UINT256)” to “CAST(et.gas_used AS uint256)” to comply with the requirement for datatypes to be lower case.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 120-120: Datatypes must be consistently lower case.
142-142
: Standardize CAST Data Type Casing (user_txs)
Update “CAST(tx.gas_used AS UINT256)” to “CAST(tx.gas_used AS uint256)” so that datatype names follow the lowercasing guideline.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 142-142: Datatypes must be consistently lower case.
162-162
: Consistent Data Type Casing in Fee Calculation
In the final fee calculation, convert “CAST(0.3 * (...) AS UINT256)” to “CAST(0.3 * (...) AS uint256)” so that the data type is in lower case, as required by the style rules.🧰 Tools
🪛 GitHub Actions: Lint
[warning] 162-162: Datatypes must be consistently lower case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mevblocker/fees/payment_due_4005800.sql
(1 hunks)mevblocker/fees/value_per_tx_4188777.sql
(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
mevblocker/fees/value_per_tx_4188777.sql
[warning] 23-23: 'UNION [DISTINCT|ALL]' is preferred over just 'UNION'.
[warning] 24-24: Function names must be consistently upper case.
[warning] 24-24: Keywords must be consistently upper case.
[warning] 61-61: Datatypes must be consistently lower case.
[warning] 62-62: Datatypes must be consistently lower case.
[warning] 71-71: Datatypes must be consistently lower case.
[warning] 120-120: Datatypes must be consistently lower case.
[warning] 142-142: Datatypes must be consistently lower case.
[warning] 147-147: Datatypes must be consistently lower case.
[warning] 162-162: Datatypes must be consistently lower case.
🔇 Additional comments (2)
mevblocker/fees/payment_due_4005800.sql (2)
17-20
: Update in final_fee CTE – Full Fee Utilization
The temporary fee reduction has been removed by selecting the full avg_block_fee_wei value. Please verify that downstream calculations (weekly_fee, amount_due_wei, and amount_due_eth) are now correctly aligned with the new fee structure changes.
39-45
: Final SELECT Calculation Verification
The final SELECT clause multiplies the full avg_block_fee_wei to compute weekly_fee and amount_due values. Double-check that applying the complete fee (rather than a reduced fraction) meets the intended business logic for the upcoming 30% fee increase.
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: 0
🧹 Nitpick comments (1)
mevblocker/fees/value_per_tx_4188777.sql (1)
162-162
: Adoption of the New 30% Fee Structure
The fee calculation has been updated to incorporate a hard-coded 0.3 multiplier, replacing the previous fee reduction. This adjustment meets the PR objective for the fee increase. As a recommendation for future maintainability, consider externalizing this fee rate into a configuration parameter so that further adjustments can be made with minimal code changes. Also, verify that the divisor (9) within the formula remains logically consistent under the new fee regime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mevblocker/fees/value_per_tx_4188777.sql
(6 hunks)
🔇 Additional comments (5)
mevblocker/fees/value_per_tx_4188777.sql (5)
16-28
: Broadened Transaction Scope via nonexclusive_flow CTE
The updated CTE now aggregates non-exclusive transactions from both the flashbots dataset and the mev_blocker_non_exclusive_txs table using a UNION DISTINCT. This change not only expands the inclusion criteria but also standardizes the casting by using varchar. Please verify that the expanded transaction set aligns correctly with downstream filters and performance expectations.
61-62
: Validation of JSON Extraction for Transaction Hashes
The modifications using FROM_HEX wrapped around JSON_EXTRACT (for extracting tx_1 and tx_from_1) appear correctly implemented. Ensure that the JSON structure is consistently formatted so that these extractions do not inadvertently fail during runtime.
120-120
: Consistent Data Type Casting in Kickback Transactions
The casting of et.gas_used as uint256 in the kickback_txs CTE standardizes the data type and ensures consistency with subsequent arithmetic operations. This change is appropriate provided that all related calculations expect the lower-case uint256 format.
141-142
: Standardized Casting in User Transactions Calculation
The expression for user_tip_wei now uses CAST(tx.gas_used AS uint256), which harmonizes the data type usage across the script. Confirm that this update does not lead to any unintended side effects in the fee calculations elsewhere.
146-147
: Updated Filter Condition Utilizing nonexclusive_flow
Changing the filtering condition to use "CAST(tx.hash AS varchar) NOT IN (SELECT hash FROM nonexclusive_flow)" properly excludes transactions flagged as non-exclusive. Please verify that this logic aligns with overall transaction inclusion criteria and no valid user transactions are mistakenly filtered out.
The query looks good. |
Looks good to me |
191d162
to
f9c03ed
Compare
This PR is a follow up to #132, it
Note, that this PR needs to be merged before next Wednesday to take effect in the payouts.
Summary by CodeRabbit