Conversation
cburgdorf
force-pushed
the
log_struct
branch
2 times, most recently
from
2b41655 to
44a5a62
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR adds proper EVM event logging support for Fe by introducing #[event] attribute for structs and #[indexed] attribute for fields. The implementation automatically generates impl Event trait implementations at HIR lowering time, computes event signature hashes at compile-time via CTFE keccak, and properly ABI-encodes non-indexed fields.
Changes:
- Introduces
#[event]and#[indexed]attributes with comprehensive validation (max 3 indexed fields, no generics, type checking) - Implements automatic HIR desugaring to generate
impl std::evm::Eventtrait implementations - Updates
Log::emit()API to dispatch to proper logN opcodes based on indexed field count - Adds
emit_raw()for raw bytes logging without topics
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ingots/std/src/evm/event.fe | Defines the Event trait with TOPIC0 constant and emit method |
| ingots/std/src/evm/effects.fe | Updates Log trait with emit/emit_raw methods, removes old TODO |
| ingots/std/src/evm.fe | Exports event module |
| crates/hir/src/core/lower/event.rs | Core event lowering logic with validation and HIR generation |
| crates/hir/src/core/lower/item.rs | Integrates event detection and error reporting into item lowering |
| crates/hir/src/core/lower/mod.rs | Exports EventError types |
| crates/hir/src/core/span/*.rs | Adds EventDesugared origin tracking |
| crates/hir/src/analysis/*.rs | Adds EventLowerPass and diagnostic reporting |
| crates/common/src/diagnostics.rs | Adds EventLower diagnostic pass |
| crates/**/test_db.rs, db.rs | Registers EventLowerPass in analysis pipeline |
| crates/uitest/fixtures/ty_check/*.fe | Validation test fixtures |
| crates/hir/test_files/desugar/*.fe | HIR desugaring test fixtures |
| crates/codegen/tests/fixtures/*.fe | Codegen test fixtures |
| crates/fe/tests/fixtures/*.fe | Integration test fixtures |
| crates/fe/tests/cli_output.rs | Fixes test predicate to match "logs" substring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44a5a62d06
ℹ️ 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".
|
I'm a bit wary of trying to automatically map fe type names to solidity names for the event signature. Ideally, we'd have some solidity compatibility trait, with a From Uniswap: |
|
That's a valid point. However, forcing every event to be annotated with a manual (error-prone) signature leaves me with a bad feeling as well. What about per-field annotations? That would eliminate the errors for the happy case and still allow the user to tweak the signature to support types that we don't support. |
|
@cburgdorf On this branch: #1233, you could do something like this: (Not perfectly elegant, but it would suffice probably) use core::intrinsic::{__as_bytes, __keccak256}
use core::keccak
use core::bytes::AsBytes
use std::evm::Address
use std::evm::effects::assert
trait SolCompat {
type S: AsBytes
const SOL_TYPE: Self::S
}
impl SolCompat for u256 {
type S = String<7>
const SOL_TYPE: Self::S = "uint256"
}
impl SolCompat for Address {
type S = String<7>
const SOL_TYPE: Self::S = "address"
}
impl SolCompat for Transfer {
type S = String<8>
const SOL_TYPE: Self::S = "Transfer"
}
trait Event {
const SIG: u256
}
struct Transfer { from: Address, to: Address, val: u256 }
impl Event for Transfer {
const SIG: u256 = keccak((Transfer::SOL_TYPE, "(", Address::SOL_TYPE, ",", Address::SOL_TYPE, ",", u256::SOL_TYPE, ")"))
}
#[test]
fn transfer_sig() {
assert(
Transfer::SIG == 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef,
)
} |
Adds proper EVM event logging support for Fe. Structs marked with `#[event]`
automatically get an `impl Event` generated at HIR lowering time.
Key features:
- `#[event]` attribute marks structs as loggable events
- `#[indexed]` attribute on fields marks them as log topics (max 3)
- Event signature hash (topic0) computed at compile-time via CTFE keccak
- Non-indexed fields are ABI-encoded in the log data section
- Uses existing `evm.emit()` API - now dispatches to proper logN opcodes
Example:
```fe
#[event]
struct Transfer {
#[indexed]
from: Address,
#[indexed]
to: Address,
value: u256,
}
evm.emit(Transfer { from, to, value })
// Emits log3 with topic0=keccak("Transfer(address,address,uint256)")
Validation errors for:
- #[event] on non-struct items
- #[indexed] outside #[event] structs
- More than 3 indexed fields
- Generic #[event] structs
- Unsupported field types
|
@sbillig Ok, I rebased this on your branch and implemented something. Not polished yet. sol.fe is very...lengthy..but at least it supports all possible solidity types. |
2 participants
Adds proper EVM event logging support for Fe. Structs marked with
#[event]automatically get an
impl Eventgenerated at HIR lowering time.Key features:
#[event]attribute marks structs as loggable events#[indexed]attribute on fields marks them as log topics (max 3)evm.emit()API - now dispatches to proper logN opcodesExample:
Validation errors for: