Add all existing e2e tests as native Fe tests #1208
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a30ec99944
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Transfer { to, amount } -> bool uses (mut store) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Implement ERC20 transfer/approval effects in fixture
The ERC20 fixture’s Transfer handler just returns true without touching balances (and the same no‑op pattern is used for Approve/TransferFrom immediately below), so fe test will pass even if transfer/allowance behavior is broken. The existing ERC20 e2e harness test asserts that transfers update balances and approvals update allowances; this new native test won’t catch regressions in those paths unless these handlers actually mutate store (or the test adds checks for those side effects).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request adds native Fe test implementations for all existing e2e tests, allowing them to be tested via "fe test" instead of the contract-harness framework. The PR introduces five new test files covering fundamental smart contract functionality.
- Converts e2e tests to native Fe tests using the
#[test]attribute andEvmutilities - Adds comprehensive boilerplate code for ABI encoding of message variants
- Tests core contract functionality including storage, structs, enums, and token operations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/fe/tests/fixtures/fe_test/storage_map.fe | Tests StorageMap functionality with balances, transfers, and allowances |
| crates/fe/tests/fixtures/fe_test/shape.fe | Tests struct operations and message dispatching with Point and Square types |
| crates/fe/tests/fixtures/fe_test/erc20.fe | Tests ERC20 token implementation with basic operations and minting |
| crates/fe/tests/fixtures/fe_test/enum.fe | Tests enum pattern matching with MyOption type (Some/None variants) |
| crates/fe/tests/fixtures/fe_test/coin.fe | Tests simple coin contract with credit, transfer, and balance operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl Encode<Sol> for Erc20::Name { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | ||
| impl AbiSize for Erc20::Symbol { const ENCODED_SIZE: u256 = 0 } | ||
| impl Encode<Sol> for Erc20::Symbol { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | ||
| impl AbiSize for Erc20::Decimals { const ENCODED_SIZE: u256 = 0 } | ||
| impl Encode<Sol> for Erc20::Decimals { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | ||
| impl AbiSize for Erc20::TotalSupply { const ENCODED_SIZE: u256 = 0 } | ||
| impl Encode<Sol> for Erc20::TotalSupply { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | ||
| impl AbiSize for Erc20::BalanceOf { const ENCODED_SIZE: u256 = 32 } | ||
| impl Encode<Sol> for Erc20::BalanceOf { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { self.account.encode(e) } } | ||
| impl AbiSize for Erc20::Allowance { const ENCODED_SIZE: u256 = 64 } | ||
| impl Encode<Sol> for Erc20::Allowance { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { | ||
| self.owner.encode(e) | ||
| self.spender.encode(e) | ||
| } | ||
| } | ||
| impl AbiSize for Erc20::Transfer { const ENCODED_SIZE: u256 = 64 } | ||
| impl Encode<Sol> for Erc20::Transfer { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { | ||
| self.to.encode(e) | ||
| self.amount.encode(e) | ||
| } | ||
| } | ||
| impl AbiSize for Erc20::Approve { const ENCODED_SIZE: u256 = 64 } | ||
| impl Encode<Sol> for Erc20::Approve { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { |
There was a problem hiding this comment.
The parameter 'e' is declared as mutable with 'mut _' but the underscore prefix suggests it's unused. The 'mut' modifier should be removed since the encoder is used in the function body.
| impl Encode<Sol> for Erc20::Name { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | |
| impl AbiSize for Erc20::Symbol { const ENCODED_SIZE: u256 = 0 } | |
| impl Encode<Sol> for Erc20::Symbol { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | |
| impl AbiSize for Erc20::Decimals { const ENCODED_SIZE: u256 = 0 } | |
| impl Encode<Sol> for Erc20::Decimals { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | |
| impl AbiSize for Erc20::TotalSupply { const ENCODED_SIZE: u256 = 0 } | |
| impl Encode<Sol> for Erc20::TotalSupply { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) {} } | |
| impl AbiSize for Erc20::BalanceOf { const ENCODED_SIZE: u256 = 32 } | |
| impl Encode<Sol> for Erc20::BalanceOf { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { self.account.encode(e) } } | |
| impl AbiSize for Erc20::Allowance { const ENCODED_SIZE: u256 = 64 } | |
| impl Encode<Sol> for Erc20::Allowance { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { | |
| self.owner.encode(e) | |
| self.spender.encode(e) | |
| } | |
| } | |
| impl AbiSize for Erc20::Transfer { const ENCODED_SIZE: u256 = 64 } | |
| impl Encode<Sol> for Erc20::Transfer { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { | |
| self.to.encode(e) | |
| self.amount.encode(e) | |
| } | |
| } | |
| impl AbiSize for Erc20::Approve { const ENCODED_SIZE: u256 = 64 } | |
| impl Encode<Sol> for Erc20::Approve { fn encode<E: AbiEncoder<Sol>>(self, mut _ e: E) { | |
| impl Encode<Sol> for Erc20::Name { fn encode<E: AbiEncoder<Sol>>(self, e: E) {} } | |
| impl AbiSize for Erc20::Symbol { const ENCODED_SIZE: u256 = 0 } | |
| impl Encode<Sol> for Erc20::Symbol { fn encode<E: AbiEncoder<Sol>>(self, e: E) {} } | |
| impl AbiSize for Erc20::Decimals { const ENCODED_SIZE: u256 = 0 } | |
| impl Encode<Sol> for Erc20::Decimals { fn encode<E: AbiEncoder<Sol>>(self, e: E) {} } | |
| impl AbiSize for Erc20::TotalSupply { const ENCODED_SIZE: u256 = 0 } | |
| impl Encode<Sol> for Erc20::TotalSupply { fn encode<E: AbiEncoder<Sol>>(self, e: E) {} } | |
| impl AbiSize for Erc20::BalanceOf { const ENCODED_SIZE: u256 = 32 } | |
| impl Encode<Sol> for Erc20::BalanceOf { fn encode<E: AbiEncoder<Sol>>(self, e: E) { self.account.encode(e) } } | |
| impl AbiSize for Erc20::Allowance { const ENCODED_SIZE: u256 = 64 } | |
| impl Encode<Sol> for Erc20::Allowance { fn encode<E: AbiEncoder<Sol>>(self, e: E) { | |
| self.owner.encode(e) | |
| self.spender.encode(e) | |
| } | |
| } | |
| impl AbiSize for Erc20::Transfer { const ENCODED_SIZE: u256 = 64 } | |
| impl Encode<Sol> for Erc20::Transfer { fn encode<E: AbiEncoder<Sol>>(self, e: E) { | |
| self.to.encode(e) | |
| self.amount.encode(e) | |
| } | |
| } | |
| impl AbiSize for Erc20::Approve { const ENCODED_SIZE: u256 = 64 } | |
| impl Encode<Sol> for Erc20::Approve { fn encode<E: AbiEncoder<Sol>>(self, e: E) { |
| Transfer { to, amount } -> bool uses (mut store) { | ||
| return true | ||
| } | ||
| Approve { spender, amount } -> bool uses (mut store) { | ||
| return true | ||
| } | ||
| TransferFrom { from, to, amount } -> bool uses (mut store) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The Transfer, Approve, and TransferFrom message handlers are stub implementations that always return true without performing any actual operations. This is incomplete functionality that could lead to incorrect behavior in tests or usage. These handlers should either implement the proper logic or the test should be updated to reflect that these are intentionally unimplemented stubs.
a30ec99 to
328efc8
Compare
This adds all existing e2e tests as tests that are written in Fe and tested via "fe test". I didn't actually remove the tests from
contract-harnessbecause I think there might be value in keeping those as a second line of defense as long as maintaining them doesn't become too much of a burden. However, I think going forward we probably want to write all such tests as native fe test tests.