-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add mapping key identification to format #37
Conversation
Co-authored-by: Daniel <[email protected]>
#### Mapping key identification | ||
|
||
The value of this field (when present) is a dictionary with (some of) the following fields: | ||
* `isMappingHash`: A boolean that identifies whether the opcode is computing a hash for a mapping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big vague, but I'm assuming only a single opcode that returns the final result should be marked? I.e. you're not interested in the whole sequence of opcodes that are involved in a high-level operation of indexing a mapping, only the final step giving you the hash, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant, sorry, likely that should be clarified.
Of course, there's one big exception to that statement, which is the case of mapping computations that themselves involve a hash at an earlier step (as can happen in Vyper). In that case the earlier SHA3 instruction should also be marked, but as a prehash instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtoman you bring up a good point that we can generalize this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But consensus says we can merge this now!
A quick stab at what mapping key information might look like in the format. Intended to be open to future expansion. Figured for different mapping hash formats, it makes more sense to just have an enumeration (that can be expanded in the future) rather than trying to specify them in some sort of DSL. :P There's not just one out there, Solidity and Vyper do things a bit differently here!
(Not sure if "prefix" and "postfix" are intuitive enough, arguably those should be swapped, or they could just be renamed to something less ambiguous.)
I'm assuming this is only used on SHA3 instructions. Potentially one might want to allow some way to mark other sorts of hashes were those ever used for this purpose, but, I figure, there's no need to go addressing that unless someone actually does it, which seems unlikely.
Closes #35.