Clarify webauthn decoding from pr comments #102
Conversation
| @@ -130,8 +134,7 @@ contract SafeWebAuthnPlugin is BaseAccount { | |||
| paramLen = abi.decode(userOp.signature[i:i+dataLen], (uint256)); | |||
| i += dataLen; // advance index | |||
| // assign authenticatorData to sig splice | |||
| dataLen = paramLen;//((paramLen >> 5) + 1) << 5; // (round up to next slot) | |||
|
|
|||
| dataLen = paramLen; | |||
There was a problem hiding this comment.
Did you have any thoughts on the question at the end of this comment?
There was a problem hiding this comment.
In that linked suggestion, it moves around how the variables are used throughout. That is:
iis always the index at the start of the data,dataLenis always used as the length of data to readparamLenis the length of the parameter
So assigning dataLen to be paramLen is hopefully helpful towards self-documenting code (and if redundant should be optimised out by the compiler anyway).
Using paramLen directly in the slice is different to how the slices are used everywhere else. So even though it would not error, it is less clear as a whole.
There was a problem hiding this comment.
Thanks for clarifying, that makes sense to me and agree that seems more clear
| dataLen = paramLen - 32; // length already read | ||
| dataLen -= 2 * 2 * 32; // exclude fixed length arrays |
There was a problem hiding this comment.
I'm not sure I'm understanding this change, can you explain what's going on here and how this leads to the same value as dataLen = 4 * 32;?
There was a problem hiding this comment.
The param length is the length of all fixed bytes including the length parameter (you can add this up from the fixed params or print out to see the value).
The first line subtracts the already-read length, the next line subtract two arrays of length 2 containing 32bytes.
There was a problem hiding this comment.
As discussed in call, clarified changes and updated one of the comments
There was a problem hiding this comment.
What did you think about this comment?
There was a problem hiding this comment.
I was at a squeeze to get the vars to fit without stack overflow. Adding another call introduces additional var copies and stack usage. I didn't try it for that reason, I can try that as a new PR.
There was a problem hiding this comment.
Ok makes sense, sounds good to me
Comments from #85