forest icon indicating copy to clipboard operation
forest copied to clipboard

fix: Fix the `GasEstimateMessageGas` API

Open akaladarshi opened this issue 2 months ago • 3 comments

Summary of changes

Changes introduced in this pull request:

  • Refactor the calculation to get the gas premium
  • Fix the issues which was causing empty prices
  • Add Fee config - MaxFee 0.07 FIL

Reference issue to close (if applicable)

Closes https://github.com/ChainSafe/forest/issues/5940

Other information and links

Change checklist

  • [ ] I have performed a self-review of my own code,
  • [ ] I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • [ ] I have added tests that prove my fix is effective or that my feature works (if possible),
  • [ ] I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • New Features

    • Configurable default max fee (defaults to 0.07 FIL) applied across node and CLI.
    • RPC message responses now include message + CID together.
    • Message send options expose max_fee, msg_uuid, and maximize_fee_cap.
  • Improvements

    • Tipset-aware gas estimation with blended premium calculation and explicit fee capping.
    • ETH gas endpoints and wallet send flow updated for new estimators.
    • Expanded TokenAmount arithmetic and percent-compare utilities.
  • Bug Fixes

    • Fixed per-sender initialization during tipset message processing.
  • Tests

    • Added cross-endpoint gas validation and refreshed RPC snapshots.

✏️ Tip: You can customize this high-level summary in your review settings.

akaladarshi avatar Sep 23 '25 11:09 akaladarshi

Walkthrough

Flip per-sender gating in tipset message application; add configurable default max fee to ChainConfig; make gas estimation tipset-aware with median-premium computation and fee capping; return RPC messages flattened with their CID; expand MessageSendSpec; update wallet/tests/snapshots; extend TokenAmount arithmetic/traits.

Changes

Cohort / File(s) Summary
Chain store: message gating
src/chain/store/chain_store.rs
Invert per-block-header gating in messages_for_tipset: first-seen from now triggers actor state lookup/initialization (insert into applied/balances); subsequent messages for same sender follow sequencing and balance checks.
Config / ChainConfig: default max fee
src/cli_shared/cli/config.rs, src/daemon/context.rs, src/networks/mod.rs
Add FeeConfig { pub max_fee: TokenAmount } with Default (0.07 FIL); add default_max_fee: TokenAmount to ChainConfig; thread config.fee.max_fee into daemon ChainConfig init; network constructors initialize field to zero.
RPC gas estimation: tipset-aware & cap logic
src/rpc/methods/gas.rs, src/rpc/methods/eth.rs, src/rpc/types/mod.rs
Make gas functions tipset-aware (accept ApiTipsetKey, load tipset); introduce GasMeta and compute_gas_premium; update estimate_gas_premium, estimate_fee_cap, estimate_message_gas signatures and flows; add cap_gas_fee; expose MessageSendSpec.max_fee, msg_uuid, maximize_fee_cap.
RPC message shape: flattened with CID
src/rpc/methods/chain.rs, src/rpc/methods/gas.rs, src/tool/subcommands/api_cmd/api_compare_tests.rs, src/wallet/subcommands/wallet_cmd.rs
Add pub struct FlattenedApiMessage { message: Message, cid: Cid } (lotus_json serialized) and change ChainGetMessage / GasEstimateMessageGas to return it; callers updated to access .message.
Tests & snapshots
src/tool/subcommands/api_cmd/test_snapshots.txt, src/tool/subcommands/api_cmd/api_compare_tests.rs, src/lotus_json/signed_message.rs
Replace several snapshots and add filecoin_gasestimatemessagegas snapshot; update API compare tests to validate GasEstimateGasLimit vs GasEstimateMessageGas outputs; simplify literal paths in test data.
Shim / TokenAmount arithmetic & traits
src/shim/econ.rs
Implement numeric/arithmetic traits (One, Num, Neg), add is_within_percent, implement Add/Sub/Mul/Div/Rem for owned/borrowed combinations via macros, AddAssign/SubAssign/Signed, and add unit tests.
Minor typing/clarity
src/rpc/methods/msig.rs
Make locked_balance explicitly TokenAmount (no behavioral change).
Wallet: use flattened message
src/wallet/subcommands/wallet_cmd.rs
In Send flow, unpack .message from GasEstimateMessageGas result before performing existing validations.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant RPC as RPC (GasEstimateMessageGas)
  participant Gas as rpc/methods/gas
  participant Chain as ChainStore
  participant Cfg as ChainConfig

  Client->>RPC: request(msg, msg_spec, tsk)
  RPC->>Gas: estimate_message_gas(msg, msg_spec, tsk)
  Gas->>Chain: load_required_tipset_or_heaviest(tsk)
  Chain-->>Gas: Tipset (messages, receipts)
  Gas->>Gas: build Vec<GasMeta> from tipset messages
  Gas->>Gas: compute_gas_premium(GasMeta list)
  Gas->>Cfg: read default_max_fee
  Cfg-->>Gas: TokenAmount
  Gas->>Gas: cap_gas_fee(default_max_fee, msg, msg_spec)
  Gas-->>RPC: FlattenedApiMessage { message, CID }
  RPC-->>Client: FlattenedApiMessage

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Potential attention points:

  • Correctness and implications of inverted gating in src/chain/store/chain_store.rs.
  • Interaction between cap_gas_fee, MessageSendSpec.max_fee/maximize_fee_cap, and ChainConfig.default_max_fee.
  • RPC type changes (FlattenedApiMessage) and all callsite updates (wallet, tests).
  • Boundary handling and correctness in compute_gas_premium and new TokenAmount arithmetic trait implementations.

Suggested reviewers

  • hanabi1224
  • LesnyRumcajs
  • elmattic

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Multiple out-of-scope changes detected: TokenAmount trait implementations (One, Num, Neg), new arithmetic operators, message chain logic inversion in messages_for_tipset, FeeConfig infrastructure, and wallet command modifications appear unrelated to the stated API fix objective. Review and isolate changes to only gas estimation API refactoring and test coverage. Move unrelated changes (TokenAmount traits, chain logic, FeeConfig, wallet) to separate PRs with their own issue tracking.
Docstring Coverage ⚠️ Warning Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: Fix the GasEstimateMessageGas API' directly addresses the main objective of the PR, which is to fix issues in the GasEstimateMessageGas API implementation.
Linked Issues check ✅ Passed The PR addresses issue #5940 by refactoring gas premium calculation, fixing empty price values, and adding API conformance test coverage for GasEstimateMessageGas with snapshot validation and cross-implementation testing.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch akaladarshi/fix-estimate-msg-gas

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 23 '25 11:09 coderabbitai[bot]

@akaladarshi As a wise guy once said (some Polish dude, I think),

No green checkmark, no review; it's basically the law

elmattic avatar Oct 03 '25 10:10 elmattic

Codecov Report

:x: Patch coverage is 84.24821% with 66 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 40.41%. Comparing base (58352c4) to head (e2a5ad3). :warning: Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/tool/subcommands/api_cmd/api_compare_tests.rs 0.00% 28 Missing :warning:
src/rpc/methods/gas.rs 89.09% 24 Missing :warning:
src/shim/econ.rs 96.02% 6 Missing :warning:
src/rpc/methods/chain.rs 0.00% 3 Missing :warning:
src/rpc/methods/eth.rs 0.00% 2 Missing :warning:
src/chain/store/chain_store.rs 0.00% 1 Missing :warning:
src/rpc/methods/msig.rs 0.00% 1 Missing :warning:
src/wallet/subcommands/wallet_cmd.rs 0.00% 1 Missing :warning:
Additional details and impacted files
Files with missing lines Coverage Δ
src/cli_shared/cli/config.rs 100.00% <100.00%> (ø)
src/daemon/context.rs 65.13% <100.00%> (+0.16%) :arrow_up:
src/lotus_json/signed_message.rs 92.00% <100.00%> (ø)
src/networks/mod.rs 85.28% <100.00%> (+0.13%) :arrow_up:
src/rpc/types/mod.rs 0.00% <ø> (ø)
src/chain/store/chain_store.rs 46.61% <0.00%> (ø)
src/rpc/methods/msig.rs 0.00% <0.00%> (ø)
src/wallet/subcommands/wallet_cmd.rs 0.00% <0.00%> (ø)
src/rpc/methods/eth.rs 23.19% <0.00%> (ø)
src/rpc/methods/chain.rs 21.05% <0.00%> (-0.05%) :arrow_down:
... and 3 more

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 58352c4...e2a5ad3. Read the comment docs.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Dec 11 '25 14:12 codecov[bot]