test: add more compression ratio test cases #1216
Conversation
WalkthroughThe test suite for transaction compression ratio estimation was expanded by adding several subtests using real Ethereum transaction examples. Each subtest decodes a hex-encoded transaction, invokes the estimation function with fixed parameters, and asserts the output matches expected compression ratios. No changes were made to the core logic or public interfaces. Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rollup/fees/rollup_fee_test.go (2)
125-132: Consider validating hex decoding success.The test case correctly implements compression ratio testing for ETH transfer transactions. However,
common.Hex2Bytessilently ignores decoding errors and may return nil or partial data for malformed hex strings.Consider adding validation to ensure the hex string decodes successfully:
- data := common.Hex2Bytes("02f86b83082750830461a40183830fb782523f94802b65b5d9016621e66003aed0b16615093f328b8080c001a0a1fa6bbede5ae355eaec83fdcda65eab240476895e649576552850de726596cca0424eb1f5221865817b270d85caf8611d35ea6d7c2e86c9c31af5c06df04a2587") + hexStr := "02f86b83082750830461a40183830fb782523f94802b65b5d9016621e66003aed0b16615093f328b8080c001a0a1fa6bbede5ae355eaec83fdcda65eab240476895e649576552850de726596cca0424eb1f5221865817b270d85caf8611d35ea6d7c2e86c9c31af5c06df04a2587" + data := common.Hex2Bytes(hexStr) + assert.NotEmpty(t, data, "Failed to decode transaction hex string")
124-177: Consider parameterizing gas price and base fee values.All test cases use identical fixed values for gas price (1000000) and base fee (1700000000). Consider if these should vary based on the actual network conditions when these transactions occurred, or if they should be parameterized for better test maintainability.
Consider creating constants or a helper function to make the test parameters more explicit:
+const ( + testGasPrice = 1000000 + testBaseFee = 1700000000 +) + t.Run("eth-transfer", func(t *testing.T) { // https://scrollscan.com/tx/0x8c7eba9a56e25c4402a1d9fdbe6fbe70e6f6f89484b2e4f5c329258a924193b4 data := common.Hex2Bytes("02f86b83082750830461a40183830fb782523f94802b65b5d9016621e66003aed0b16615093f328b8080c001a0a1fa6bbede5ae355eaec83fdcda65eab240476895e649576552850de726596cca0424eb1f5221865817b270d85caf8611d35ea6d7c2e86c9c31af5c06df04a2587") - ratio, err := estimateTxCompressionRatio(data, 1000000, 1700000000, params.TestChainConfig) + ratio, err := estimateTxCompressionRatio(data, testGasPrice, testBaseFee, params.TestChainConfig)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/fees/rollup_fee_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
rollup/fees/rollup_fee_test.go (1)
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
🧬 Code Graph Analysis (1)
rollup/fees/rollup_fee_test.go (2)
common/bytes.go (1)
Hex2Bytes(79-82)params/config.go (1)
TestChainConfig(501-541)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
rollup/fees/rollup_fee_test.go (6)
9-9: LGTM: Import addition is appropriate.The
commonpackage import is correctly added to support theHex2Bytesfunction used in the new test cases.
134-141: LGTM: SCR transfer test case is well-implemented.The test case correctly validates compression ratio estimation for SCR token transfers with appropriate expected ratio of 1.1x.
143-150: LGTM: SyncSwap test demonstrates good compression.The test case validates a complex SyncSwap transaction with high compression ratio (3.1x), which is expected given the repetitive patterns in swap contract interactions.
152-159: LGTM: Uniswap test case covers important DeFi scenario.The test case appropriately validates compression ratio for Uniswap V3 swap transactions with expected 1.7x compression ratio.
161-168: LGTM: EtherFi deposit test covers staking scenario.The test case correctly validates compression ratio for liquid staking deposits with appropriate 1.4x expected compression ratio.
170-177: LGTM: Oracle update test covers important infrastructure scenario.The test case validates compression ratio for EdgePushOracle post updates with reasonable 2.1x compression ratio, covering an important infrastructure transaction type.
2 participants
1. Purpose or design rationale of this PR
Test cases from recent transactions on Scroll mainnet.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit