-
Notifications
You must be signed in to change notification settings - Fork 151
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
fix: /accounts/{accountId}/convert
encoding for ecdsa
#1280
Conversation
/accounts/{accountId}/convert
endpoint/accounts/{accountId}/convert
encoding for ecdsa
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.
looks good!
@@ -158,7 +179,7 @@ describe('Convert accounts', () => { | |||
const expectedResponse = { |
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.
Old Code
By reading the comments above this test, it was giving as output an invalid address on purpose.
The comments say that the user :
- gives to the endpoint a non public key but
- says/states falsely to the endpoint that it is a public key (by setting query param
publicKey=true
)
and then the output is/should be an invalid address. So that was with the old code.
New Code
With the new code, the output is correct even if the user states that the input is a public key when its not. So in the case of scheme=ecdsa
, if the user sets query param publicKey
either true
or false
it will go to this new line of code :
ecdsa: (p: string) => (hexToU8a(p).length > 32 ? blake2AsHex(p) : p),
and it will encode correctly despite the value of publicKey
query param.
Thoughts
So, some thoughts based on the above :
- In retrospective, since I was the one who added the original test, I have to admit that it is a little bit confusing and not sure how useful it is anymore. I think my goal was to "catch" all possible combinations of inputs.
- However, if we keep this test with the new code, I think we should change the comments so that they reflect what the test is doing right now.
- Also, by checking the new code, I am not sure if the query param
publicKey
is needed anymore. Wdyt @TarikGul ? Since it will convert it correctly by checking the length (in theecdsa
case)
cc @IkerAlus Thank you for your time on trying to check this. It really helped on clearing my head and checking efficiently after our talk.
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.
I am not sure if the query param publicKey is needed anymore. Wdyt @TarikGul ? Since it will convert it correctly by checking the length (in the ecdsa case)
I agree that the publicKey
query param is not needed anymore. I would say we keep this PR small to first fix the way we deal with ecdsa
then we can have some follow up work to remove the publicKey
query param.
However, if we keep this test with the new code, I think we should change the comments so that they reflect what the test is doing right now.
👍 Sounds good.
…into tarik-fix-convert
This ensures that we only hash the accountId or key given for a ecdsa scheme type.