feat: minor improvements to Ethereum delegated siggys #10084
Conversation
| } | ||
| } | ||
|
|
||
| if paramsReader.Len() != 0 { | ||
| return EthTxArgs{}, xerrors.Errorf("extra data found in params") | ||
| } | ||
|
|
||
| if len(params) == 0 && msg.Method != builtintypes.MethodSend { |
There was a problem hiding this comment.
@Stebalien I think we need this to cover a case where the EthTx calls the EAM's CreateExternal method with the CBOR-encoding of the empty byte array, which would fail to roundtrip.
There was a problem hiding this comment.
Feels cleaner regardless.
There was a problem hiding this comment.
Yes, but isn't that case already covered? I.e., we should fail to read a bytes array from the params in that case. But either way works.
There was a problem hiding this comment.
@Stebalien So that's what I was unsure about it, but I think this case wasn't covered? You could have successfully read (the serialization of) the empty byte array -- [MajByteString, 0], I think, which would still lead to params having a length of 0. And that wouldn't roundtrip because we assert non-empty bytes for CreateExternal calls when going Eth -> Fil.
| } | ||
| } | ||
|
|
||
| if paramsReader.Len() != 0 { | ||
| return EthTxArgs{}, xerrors.Errorf("extra data found in params") | ||
| } | ||
|
|
||
| if len(params) == 0 && msg.Method != builtintypes.MethodSend { |
There was a problem hiding this comment.
Yes, but isn't that case already covered? I.e., we should fail to read a bytes array from the params in that case. But either way works.
Related Issues
Minor improvements noticed while writing this logic into FIP-0055.
Proposed Changes
ReadByteArray/WriteByteArrayAdditional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps