fix[lang]: fix allowing un-instantiable events - #5206
Conversation
Gas ChangesNo changes detected. Summary
|
![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: 1df785bc96
ℹ️ 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".
| else: | ||
| # TODO block at higher level | ||
| raise TypeMismatch("Event indexes may only be value types", expr) | ||
| raise CodegenPanic("Event indexes may only be value types", expr) |
There was a problem hiding this comment.
When logging an event imported from a JSON ABI, EventT.from_abi() bypasses the new from_EventDef validation and preserves indexed on array/struct inputs, so this branch is still reachable for inputs like an ABI event E(uint256[3] indexed a). Changing it to CodegenPanic turns a bad user-supplied ABI/use-site into an internal compiler error report instead of the previous TypeMismatch; either validate ABI events before codegen or keep this as a user-facing exception in both backends.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@charles-cooper is this likely ?
Where would an invalid JSON ABI come from ?
And if it's possible, shouldn't we error at import time instead ?
There was a problem hiding this comment.
yes we should validate at import time
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d60162ff
ℹ️ 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".
| typ = type_from_annotation(annotation.args[0]) | ||
| can_be_indexed = typ._is_prim_word or typ._is_bytestring | ||
| if not can_be_indexed: | ||
| raise TypeMismatch("Event indexes may only be value types", annotation) |
There was a problem hiding this comment.
This validation runs for every EventDef, including .vyi/inline interface events and contract events that are only exported in the ABI, before any log codegen is involved. As a result, a valid ABI/source interface such as event E: a: indexed(uint256[3]) is rejected even though the same event still imports from JSON via EventT.from_abi() and can be part of an external contract interface; the legacy use-site limitation does not justify rejecting ABI declarations that are never emitted. Please either limit this check to events that are actually logged, keeping a user-facing error at that use site, or make source interfaces match JSON ABI handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That is the point of this PR
| validate_call_args(annotation, 1) | ||
|
|
||
| typ = type_from_annotation(annotation.args[0]) | ||
| can_be_indexed = typ._is_prim_word or typ._is_bytestring |
There was a problem hiding this comment.
this should just be a property on the type typ._can_be_index, also i think we already have some property like this somewhere?
There was a problem hiding this comment.
You're right there was _as_hashmap_key, let me know what you think of ef34b9a
There was a problem hiding this comment.
keep _as_hashmap_key and add a property _as_event_index which resolves to _as_hashmap_key, e.g.
@property
def _as_event_index(self):
return self._as_hashmap_key
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfc01e9d01
ℹ️ 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".
| if indexed_count > 3: | ||
| raise EventDeclarationException( | ||
| "Event cannot have more than three indexed arguments", decl_node | ||
| ) |
There was a problem hiding this comment.
This constructor is also reached by EventT.from_abi(), but from_abi() discards the ABI's anonymous flag before this new unconditional three-index limit is applied. When a valid Solidity JSON ABI contains an anonymous event with four indexed parameters, importing that ABI now raises EventDeclarationException and blocks the interface even if the Vyper contract only needs its functions; anonymous events have no signature topic, so they can use all four log topics. Please carry the anonymous flag into this validation or keep the three-index limit only for non-anonymous/Vyper-declared events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 893f34b3ca
ℹ️ 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".
| members[item["name"]] = type_from_abi(item) | ||
| return cls(abi["name"], members, indexed) | ||
|
|
||
| return cls(abi["name"], members, indexed, is_anonymous=abi.get("anonymous", False)) |
There was a problem hiding this comment.
When the imported ABI has anonymous: true, this line only passes the flag into __init__ for the indexed-count check; EventT never stores it, while both log-lowering paths still always prepend event.event_id as the first topic. The fresh evidence is the new test_json_abi_anonymous_four_indexed_log: for an anonymous event with four indexed args, validation now accepts it, but emitting it builds five topics and hits the len(topics) <= 4 assertion or produces a non-anonymous log instead of four indexed topics. Please persist the anonymous flag on EventT and skip the signature topic in both backends.
Useful? React with 👍 / 👎.
2 participants
What I did
Fix #5137
How I did it
Catch the invalid types in
EventT.from_EventDefConvert the type mismatch at use-site to a
CodegenPanicHow to verify it
pytest, includes new testsCommit message
Description for the changelog
Fix being allowed to create events which are then un-instantiable
Cute Animal Picture