feat: add method_id member to interface functions#4903
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5dcdff3b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if attr in self.functions: | ||
| return TYPE_T(self.functions[attr]) |
There was a problem hiding this comment.
Reject interface function types in annotations
Exposing interface functions from get_type_member makes IFoo.transfer parse as a concrete annotation type via type_from_annotation(...), even though ContractFunctionT is not a valid ABI/storage type. As a result, declarations such as x: public(IFoo.transfer) can get past type parsing and then fail later when ABI/getter generation needs to_abi_arg (function types do not implement abi_type), producing an internal compiler failure instead of a user-facing type error. This member should be gated to expression use (...method_id) and not accepted as a general annotation type.
Useful? React with 👍 / 👎.
charles-cooper
left a comment
There was a problem hiding this comment.
i think maybe it makes sense for the evaluation to happen during constant folding? cc @Sporarum
|
As for constant folding, I guess it technically is a compile-time constant, but I would not expect constant folding to be doing that job We could instead move the computation to something like Also see my comments on the VIP: #2098 (comment) |
| return VyperValue.from_stack_op(IRLiteral(value), typ) | ||
|
|
||
| # Case 2: Address properties | ||
| # Case 1b: Interface.function.method_id (e.g. ERC20.transfer.method_id) |
There was a problem hiding this comment.
Case 1 should be modified to Case 1a then
Done here 9f51422
Re default args, current implementation uses the full signature (all args). Happy to change if a different behavior is preferred Re naming ( |
|
cf. #2098 (comment) on functions with default args |
|
First commit defaulted This way If |
|
do we want to handle things like |
ok i think we can actually handle this as a follow up. the PR looks good to me, @Sporarum do you have any more comments? |
Sporarum
left a comment
There was a problem hiding this comment.
@charles-cooper I think we should not default to 0, and instead force the user to specify the count if there are kwargs: #2098 (comment)
And a couple small comments:
| builtin = MethodIDOf() | ||
| selector = builtin._compute_method_id(node) |
There was a problem hiding this comment.
| builtin = MethodIDOf() | |
| selector = builtin._compute_method_id(node) | |
| selector = MethodIDOf()._compute_method_id(node) |
| @@ -443,6 +443,10 @@ def infer_kwarg_types(self, node): | |||
| # dispatch into get_type_member if it's dereferenced, ex. | |||
| # MyFlag.FOO | |||
| def get_member(self, key, node): | |||
There was a problem hiding this comment.
Why is this change required ?
| # get an event, struct or flag from this interface | ||
| return TYPE_T(self._helper.get_member(attr, node)) | ||
|
|
||
| def get_member_in_expr(self, attr, node): |
There was a problem hiding this comment.
I think this function name is confusing, I don't see what it semantically means
| for kw in node.keywords: | ||
| if kw.arg == "n_optional_args": |
There was a problem hiding this comment.
This seems a bit convoluted, wouldn't node.keywords.get("n_optional_args", 0) or something like it work ?
How do the other built-ins deal with kwargs ?
| # default: 0 optional args (positional only) | ||
| return 0 | ||
|
|
||
| def _compute_method_id(self, node): |
There was a problem hiding this comment.
Is this logic a duplicate ?
It seems like we would already have something to compute that
What I did
Implemented VIP #2098 - added a
method_id_ofbuiltin function, allowing direct access to the 4-byte function selector viamethod_id_of(Interface.function).How I did it
InterfaceT.get_member_in_exprexposes functions in expression context (gated from annotation path to prevent use as type)method_id_ofbuiltin inbuiltins/functions.pywith build_IR for legacy codegencodegen_venom/builtins/misc.pyn_optional_argskwarg to specify how many default args to include in the signature (defaults to0, positional-only)How to verify it
Commit message
Description for the changelog
Added
method_id_ofbuiltin function, enablingmethod_id_of(ERC20.transfer)syntax to retrieve the 4-byte function selector as bytes4.Cute Animal Picture