KS-35: EVM Encoder compatible with consensus capability #12025
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
bolekk
force-pushed
the
feature/KS-35-encoder
branch
from
3358a02 to
d8fec25
Compare
| if !ok { | ||
| return nil, fmt.Errorf("expected input to be a map") | ||
| } |
There was a problem hiding this comment.
Is this even possible? You have Map as the input. I'm not sure I would want to trust the system anymore if that doesn't produce a map[string]any
| return nil, err | ||
| } | ||
| unwrappedMap[InnerUserDataFieldName] = userPayload | ||
| return c.codec.Encode(ctx, unwrappedMap, outerEncoderName) |
There was a problem hiding this comment.
I'm a bit lost, it looks like you're setting only one of the three fields that needs to get encoded
There was a problem hiding this comment.
The other two are already there.
In any case, I will rework this part after discussing a slightly more gas-efficient way to pack IDs with Blaz.
bolekk
force-pushed
the
feature/KS-35-encoder
branch
from
d8fec25 to
e71a16f
Compare
|
| rest = rest[1:] | ||
| } | ||
| } | ||
| if len(rest) == 0 || rest[0] != ')' { |
There was a problem hiding this comment.
The if statements here are a little awkward. Not going to necessarily request changes, but I would suggest something simpler like if !(len(rest) == 1 && rest[0] == ')')
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* develop: KS-35: EVM Encoder compatible with consensus capability (#12025) (feat): Add ability for Functions ToS migrations (#11827) FUN-1247 (refactor): Move Functions Coordinator duplicate request ID check earlier (#12018) Add plugins build back into CI (#12054) chore: github action version bumps (#12023) Bump libocr to fe2ba71b2f0a0b66f95256426fb6b8e2957e1af4 (#12049) Pin to latest version chainlink-common (#12053) Node API capabilities registry (#12046)
| // and consumed by other functions in this package. | ||
| // Note, although uppercase letters are not part of the ABI spec, this function | ||
| // still accepts it as the general format is valid. | ||
| func ParseSignature(unescapedSelector string) (abi.SelectorMarshaling, error) { |
There was a problem hiding this comment.
Why is this named unescpaedSelector? I can't find an "escaped" version. What does it need to be unescaped from?
There was a problem hiding this comment.
I modified this verbatim from geth (ParseSelector being the original) so I retained the naming
There was a problem hiding this comment.
it's mostly just because of naming here so the variable names don't conflict:
| // and consumed by other functions in this package. | ||
| // Note, although uppercase letters are not part of the ABI spec, this function | ||
| // still accepts it as the general format is valid. | ||
| func ParseSelector(unescapedSelector string) (abi.SelectorMarshaling, error) { |
There was a problem hiding this comment.
What's the difference between ParseSelector and ParseSignature (below)? Seems like ParseSelector is not used anywhere.
There was a problem hiding this comment.
@DeividasK , @austinborn these two files were copied from geth (see comment above: // Sourced from https://github.com/ethereum/go-ethereum/blob/fe91d476ba3e29316b6dc99b6efd4a571481d888/accounts/abi/selector_parser.go#L126
// Modified assembleArgs to retain argument names)
Blaz modified them slightly to include argument name parsing as geth by default doesn't support that. They happened to be merged with my PR because encoders also depend on this library and were merged earlier.
We can either keep them as close to original as possible or make cleaner/smaller (I already had to do minor tweaks to satisfy Sonar). I'll leave that decision to @archseer and @patrick-dowell .
No description provided.