Skip to content

feat(target_chains/ethereum): add tx fee to evm contract #2526

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

Merged
merged 9 commits into from
Mar 31, 2025
Merged

Conversation

cctdaniel
Copy link
Contributor

@cctdaniel cctdaniel commented Mar 27, 2025

Summary

Added support for transaction-level fees in Pyth price feed updates by:

  • Introducing a new governance action SetTransactionFee (action type 8)
  • Adding transaction fee state management in the Pyth contract
  • Modifying fee calculation to include both per-update and per-transaction fees

Rationale

These changes enable charging a base transaction fee in addition to per-update fees for price feed updates. This helps:

  • Better align fee structure with actual gas costs by having a fixed component per transaction
  • Provide more flexible fee management through governance controls
  • Create a more sustainable economic model for price feed updates

Technical Changes

  1. Contract Changes:

    • Added transactionFeeInWei state variable to PythStorage
    • Modified getTotalFee() to include transaction fee component
    • Added getter/setter functions for transaction fee management
    • Implemented governance action handler for SetTransactionFee
  2. Governance Support:

    • Added new SetTransactionFee action type and payload structure
    • Implemented payload parsing and validation
    • Added event emission for fee changes
    • Extended governance instruction set with transaction fee management
  3. TypeScript Changes:

    • Added SetTransactionFee governance action implementation
    • Updated action type enums and decoders
    • Added buffer layout for fee value and exponent encoding

How has this been tested?

  • Current tests cover my changes

    • Existing fee calculation tests continue to pass
    • Governance action handling tests remain valid
  • Added new tests

    • Unit tests for transaction fee parsing and validation
    • Integration tests for fee calculation with transaction component
    • Governance action tests for SetTransactionFee
  • Manually tested the code

    • Verified fee calculations with various update counts
    • Tested governance action execution flow
    • Validated event emission on fee changes

Copy link

vercel bot commented Mar 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2025 9:33am
component-library ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2025 9:33am
entropy-debugger ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2025 9:33am
insights ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2025 9:33am
proposals ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2025 9:33am
staking ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2025 9:33am

@ali-bahjati ali-bahjati requested a review from Copilot March 28, 2025 10:09
Copy link

@Copilot Copilot AI left a 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 introduces transaction-level fee support for Ethereum contracts in the Pyth price feed updates through new governance actions and contract state changes.
Key changes include:

  • Implementation of a new governance action "SetTransactionFee" with corresponding payload structure and encoding/decoding logic.
  • Contract modifications to manage the transaction fee state and update fee calculations.
  • Updates to governance payload tests to cover serialization and deserialization of the new action.

Reviewed Changes

Copilot reviewed 5 out of 11 changed files in this pull request and generated no comments.

File Description
governance_payload/index.ts Added import, case handling, and export for SetTransactionFee in the governance API.
governance_payload/SetTransactionFee.ts Implemented the SetTransactionFee class with encode/decode functionality.
governance_payload/PythGovernanceAction.ts Defined the new action type (value 8) for SetTransactionFee.
tests/GovernancePayload.test.ts Added tests for serializing/deserializing the new SetTransactionFee action.
Files not reviewed (6)
  • target_chains/ethereum/contracts/contracts/pyth/Pyth.sol: Language not supported
  • target_chains/ethereum/contracts/contracts/pyth/PythGetters.sol: Language not supported
  • target_chains/ethereum/contracts/contracts/pyth/PythGovernance.sol: Language not supported
  • target_chains/ethereum/contracts/contracts/pyth/PythGovernanceInstructions.sol: Language not supported
  • target_chains/ethereum/contracts/contracts/pyth/PythSetters.sol: Language not supported
  • target_chains/ethereum/contracts/contracts/pyth/PythState.sol: Language not supported
Comments suppressed due to low confidence (1)

governance/xc_admin/packages/xc_admin_common/src/tests/GovernancePayload.test.ts:429

  • [nitpick] Consider renaming record keys 'v' and 'e' to 'newFeeValue' and 'newFeeExpo' for improved clarity in the tests.
return fc.record({ v: fc.bigUintN(64), e: fc.bigUintN(64) })

Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

Nice! and thanks for moving the tests to foundry. Please address my comments. Also can you update contract version so we don't miss it? (you can use a alpha next version because we are going to have more changes there)

@cctdaniel cctdaniel requested a review from a team as a code owner March 31, 2025 09:31
@cctdaniel cctdaniel merged commit b8f388e into main Mar 31, 2025
11 checks passed
@cctdaniel cctdaniel deleted the pyth-tx-fee branch March 31, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants