txnbuild: update challenge tx helpers for SEP-10 v1.3.0 #2071
Conversation
There was a problem hiding this comment.
LGTM! Would you also add a changelog update in the "unreleased" section?
leighmcculloch
dismissed
ire-and-curses’s stale review
I'm planning on making significant changes to this PR.
|
@msfeldstein provided some feedback to me directly that we need to be able to offer this as a similar level of drop-in as the prior functions that support SEP-10. In light of this I think I'm going to move the functions I'm adding out of |
leighmcculloch
changed the title
|
@ire-and-curses Sorry, I fixed one bug since you last 👀d that I don't think I could merge and fix separately and is worth a second set of eyes. See 99529e7. |
(cherry picked from commit edecbbd)
|
|
…o ensure that the code never allows a signature to be consumed more than once, and as a side effect of that some of the guards and checks had to move, which I felt made this require more comments also. The comments might have been required anyway really.
Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com>
…d add some code to ensure the server key is rematched when we're matching the client keys. This is a very unlikely thing protecting against.
|
Sorry @ire-and-curses I'd pushed the restructure yesterday, but I hadn't finished cleaning up some things that showed up during it. I just pushed those changes. I think this is it unless you have any concerns. |
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
I've updated any docs (developer docs,.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Add new challenge transactions helpers to
txnbuildto support verifying a challenge transaction that has multiple signatures. Add three new functions:ReadChallengeTx: Read the details like the transaction and the client account ID out of the transaction.VerifyChallengeTxSigners: Verify that the signers of the transaction are a subset of a list of signers given to the function.VerifyChallengeTxThreshold: Verify that the signers of the transaction are a subset of a list of signers that meet a required threshold.Deprecate
VerifyChallengeTx.Why
The SEP-10 change that is open in stellar/stellar-protocol#489 clarifies how an implementer should verify that signers of the transaction are signers on the account and that accounts may have multiple signers.
An implementer needs to read out of the transaction the client account before verifying the transaction. For this reason we need two functions, one to read out details we need before verification and one to perform verification.
The read call also validates the server signature because no challenge transaction would ever be valid to read if it wasn't signed by the server.
Known limitations
These helper functions take a decent amount of complexity away from an implementer, but they still leave a decent amount up to the implementer. An implementer would still need to make a call to Horizon. I think it might be out-of-scope of the
txnbuildpackage to offer that additional logic and SEP-10 technically leaves the exact details of what happens there up to the implementer so it would also be difficult to implementer in a library function.