feat: add support for SEP-0010. #264
Conversation
overcat
changed the title
|
@overcat thank you for your contribution! I think if you use Sets and Maps instead of List the code will be simpler. Let me know what you think. |
### What Ignore txhash (T...) signers, used for preauth transactions, xhash (X...) signers, and other future types of signers the server might not know about, during SEP-10 verification. ### Why Developers will likely pass through to the verification functions the signers on accounts as provided by Horizon. Accounts can have other non-ed25519 signers and they're likely going to be passed through verbatim. The verification logic's goal is to confirm the transaction has been signed by the signers and so ignoring unsupported types like txhash and xhash seems like a safe thing to do given that the verification function will also ignore ed25519 signers that don't match a signature. Without this in a typical SEP-10 implementation any account with a txhash or xhash signer will likely fail SEP-10 verification. Issues that might be caused by this new behavior is if a user passes in an account seed (S...) or some other string they won't see an error. I think that's unlikely and hopefully a smaller impact than is worth making this solution more complex. This issue was first identified by @overcat in lightsail-network/java-stellar-sdk#264, but solved in a way that depends on data from Horizon. This solution does not depend on data from Horizon and should be portable to all our SDKs. This was previously discussed at: lightsail-network/java-stellar-sdk#264 (comment).
There was a problem hiding this comment.
@overcat great job, this looks good to me! I will defer to @leighmcculloch for the final approval before we can merge
@tamirms Thanks, I've got it in my queue to do a final pass over this PR and I hope to get to it before the end of this week. @overcat Before I do a final pass, I noticed that during my first pass the number of tests seemed smaller than the number of tests we have in the Go SDK. We need to make sure we're covering at least the same test scenarios that are there. I haven't dug into the specifics of the tests here yet, but are there less tests because you combined some of the scenarios or are we missing some? |
|
Hi, @leighmcculloch, thank you for your reminder, I will add more tests later. When I checked the test code of Golang, I found that the behavior of TestReadChallengeTx_invalidNotSignedByServer was different from what I expected. This should be a bug, maybe this line should be changed to |
Good catch, that if statement is guarding on the event someone passes a
Thanks! Please mention me when you've added the tests and I'll take another look, and feel free to mention me if you see any other issues with the tests too. Thanks! |
|
Hi @leighmcculloch, I added more tests with reference to the Golang implementation, but I did not add the following tests:
BTW, I think this line should be modified to |
|
@overcat Thanks for reporting the unnecessary signer in that test, I'll get that fixed up in stellar/go#2279. I agree you don't need to add the duplicate signers test this the language protects against that with the use of a Set. 🎉 Thanks for being so thorough with the addition of more tests. I need a couple days to go over them and to review the questions about the other two tests that you left out. Look out for a follow up from me next week. Thanks! |
There was a problem hiding this comment.
@overcat I reviewed all the new tests, and they look awesome. I love that you used the same order as in the Go SDK as it made reviewing the tests so much simpler ❤️. We're very close now to merging this. I've left some inline comments below.
💡 – Two ideas for how we could make this easier to use and optimal, but that would be entirely building on what is here, so I think we could keep them for a separate PR once this has merged so we can get this merged sooner. Feel free to ignore these.
👀 – @tamirms Would love your 👀 on this one.
❗️ – A couple very small mistakes to fix up (test names got mixed up I think 😬). Plus I had a look at the two tests you asked about (TestReadChallengeTx_invalidCorrupted, TestReadChallengeTx_invalidDataValueCorruptBase64) and I think we should add them as tests. The code doesn't need to catch a base64 encoding error, we just want to capture those failure scenarios and make sure that if the code behaves one way today when invalid base64 is provided that it continues to behave the same way as the code evolves and is maintained.
| weightsForSigner.put(signer.getKey(), signer.getWeight()); | ||
| } | ||
|
|
||
| Set<String> signersFound = verifyChallengeTransactionSigners(challengeXdr, serverAccountId, network, weightsForSigner.keySet()); |
There was a problem hiding this comment.
💡 Any reason we're passing in weightsForSigner.keySet() here and not just signers? It results in creating another copy of the set in memory. This doesn't need to block this PR, but it might be an area to optimize later if it isn't being done for a particular reason.
There was a problem hiding this comment.
Because in my opinion, the weight of the signer is completely useless for verifyChallengeTransactionSigners, so I don't think it is necessary to pass it in. Of course, memory consumption is a noteworthy point, thank you for your reminder.
There was a problem hiding this comment.
Agreed, but you can pass in the signers value which is one of the function parameters and is a Set<String>.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🎉 Thanks for the huge effort and thorough testing @overcat !
|
Thank you both for your hard work, you have really given me a lot of help, and I have gained a lot here. Thank you sincerely. ❤️ |
This resolves #263 and resolves #235.
Feel free to add comments if you have any questions.
Thanks.