SEP-10 Multisig Support by JakeUrban · Pull Request #69 · astroband/ruby-stellar-sdk

JakeUrban · 2020-03-10T23:37:47Z

resolves #68

Changes Summary:

Added the Stellar::SEP10 class, methods:
- build_challenge_tx (moved from Stellar::Client)
- read_challenge_tx (renamed from Stellar::Client.verify_challenge_tx)
- verify_challenge_tx_signers
- verify_challenge_tx_threshold
- verify_challenge_tx
- verify_tx_signatures
- verify_tx_signed_by
build_challenge_tx, verify_challenge_tx, and verify_tx_signed_by, from Stellar::Client are now wrappers around the same functionality implemented in the SEP10 class.
added tests for multisig functionality

JakeUrban · 2020-03-12T19:19:02Z

@jimdanz I can't add you as a reviewer but you're welcome to take a look

msfeldstein · 2020-03-12T20:32:11Z

lib/stellar/sep10.rb

+      end
+
+      # Confirm all signatures were consumed by a signer.
+      if signers_found.length != te.signatures.length - 1


Does this allow for someone to sign multiple times with the same client key?

We remove duplicate signatures from the returned list, so no

msfeldstein

Approved after putting the dedupe back and a test for it

examples/07_sep10.rb

spec/lib/stellar/sep10_spec.rb

jimdanz · 2020-03-13T00:53:46Z

👍

…tibility

leighmcculloch

I haven't worked my way through the tests yet, but I looked through the non-test code. Highly recommend modeling the new functions and all the tests off the Go SDK as it more robustly deals performs signature verification and the tests cover lots of cases. There's a few places I've left a comment at that don't look like they cover all the cases that we handle in other SDKs, especially in regards to verifying signatures. Anything we can do to keep this similar to the Go SDK will help us maintain it too.

README.md

lib/stellar/account_signer.rb

lib/stellar/client.rb

lib/stellar/sep10.rb

lib/stellar/client.rb

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

JakeUrban · 2020-03-16T23:57:07Z

Should be good for another round of review @leighmcculloch 👍

This reverts commit b8c8212.

leighmcculloch

There's a couple issues with the logic verifying signatures. There are also a few scenarios that aren't tested that it would be great to test. The Go and Java SDKs have a good set of test scenarios to follow as a reference that exercise all the weird and wonderful ways a transaction might be setup and how we'd want to behave.

lib/stellar/sep10.rb

spec/lib/stellar/sep10_spec.rb

leighmcculloch · 2020-03-19T00:38:12Z

spec/lib/stellar/sep10_spec.rb

+    end
+  end
+
+  describe "#verify_challenge_tx_threshold" do


Could we add tests for these use cases?

not signed by server (error case)

signed by server and clients all signatures used but some signers aren't used (valid case) (Go SDK ref)

signed by server and clients all signatures used but some PreauthTxHash (T signer) and XHash (X signer) signers ignored (valid case) (Go SDK ref)

signatures unrecognized and can't be mapped to a signer (error case) (Go SDK ref)

no signers (error case)

no signatures (error case)

All of these tests test functionality defined in other functions.

verify_challenge_tx_threshold is essentially a call to verify_challenge_tx_signers with the additional functionality of checking threshold. I guess this is more a question of testing philosophy, but I don't see the value in adding these tests for this function.

Even if verify_challenge_tx_threshold no longer called verify_challenge_tx_signers down the line, whatever it did call instead would presumably also have tests for its functionality. If we moved the implementation of said functionality into verify_challenge_tx_threshold, thats when it would be appropriate to add the unit tests there. In my opinion unit tests should be applied to wherever the functionality is implemented.

I often take the approach you describe, but the reason we were so thorough with tests in the other SDKs is because these functions are handling signature verification and authentication, so there is a lot at stake. The tests I listed are tests we have in the other SDKs.

I missed asking for two tests on the _threshold function, which it looks like we also missed in the Go SDK, and strangely enough would catch a double counting of weight bug that I think exists in this function:

duplicate signers

duplicate signatures

lib/stellar/sep10.rb

spec/lib/stellar/sep10_spec.rb

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

leighmcculloch

The new tests look great 🎉 . I think there's a bug relating to double counting weights below and a test missing to catch that.

leighmcculloch · 2020-03-21T03:13:10Z

lib/stellar/sep10.rb

+      signers.each do |s|
+        if !signer_strs_found.include?(s['key'])
+          next
+        end
+        signers_found.add(s)
+        weight += s['weight']
+      end


I think these lines result in double counting a weight because it is iterating over the input signers that may be an array containing duplicates. We should have a test that catches this case, specifically duplicate signers in the input list shouldn't cause an error but should only be counted once. We have a test that captures it for the _signers function but not this function and the bug exists in this function.

I added code to remove the signer key from signer_strs_found before adding its weight to the total. So if there is a duplicate signer in signers, it will be skipped the second time it appears in the loop.

leighmcculloch · 2020-03-21T03:23:31Z

spec/lib/stellar/sep10_spec.rb

+    end
+  end
+
+  describe "#verify_challenge_tx_threshold" do


I missed asking for two tests on the _threshold function, which it looks like we also missed in the Go SDK, and strangely enough would catch a double counting of weight bug that I think exists in this function:

duplicate signers

duplicate signatures

leighmcculloch

The library code looks good 🎉 . The tests just added aren't really testing the library code because they're using a Set in the test, see below. Looks great otherwise.

leighmcculloch · 2020-03-23T20:07:27Z

lib/stellar/sep10.rb

        if !signer_strs_found.include?(s['key'])
          next
+        else
+          signer_strs_found.delete(s['key'])


This is a smart way of getting around the fact that you can't index efficiently into the original signers array. 👏 When I was originally looking at this code I didn't see a good way of doing this. This is so simple, love it.

leighmcculloch · 2020-03-23T20:08:08Z

lib/stellar/sep10.rb

+        else
+          signer_strs_found.delete(s['key'])
        end
        signers_found.add(s)
        weight += s['weight']


What's the goal of having the delete inside the else? Everything else that runs in this block is essentially the else case because inside the if we call next that skips the rest of the block. Unless I'm missing something you could drop the else:

Suggested change

else

signer_strs_found.delete(s['key'])

end

signers_found.add(s)

weight += s['weight']

end

signer_strs_found.delete(s['key'])

signers_found.add(s)

weight += s['weight']

leighmcculloch · 2020-03-23T20:10:48Z

spec/lib/stellar/sep10_spec.rb

+        sep10.verify_challenge_tx_threshold(
+          challenge_xdr: challenge,
+          server: server_kp,
+          signers: Set[{'key' => client_kp.address, 'weight' => 1}],


Could you add a second version of this test that has the signers be an Array that contains the client kp twice?

Can't pass Arrays, but I'll add a second signer with the same address but different weight. This will be deduplicated before passing to verify_challenge_tx_signers though, which builds the set of addresses.

leighmcculloch · 2020-03-23T20:15:19Z

spec/lib/stellar/sep10_spec.rb

+          signers: Set[
+            {'key' => client_kp.address, 'weight' => 1},
+            {'key' => client_kp.address, 'weight' => 1}
+          ],


This test isn't actually testing duplicate signers, it's testing a single signer because the Set type will automatically deduplicate these two entries. Take a look at this repl as an example.

You should use an Array instead of a Set here so the duplicates are preserved outside of the SDK, and the SDK should be the one responsible for handling duplicates. Favor arrays in tests to ensure the SDK does the deduplication necessary.

Suggested change

signers: Set[

{'key' => client_kp.address, 'weight' => 1},

{'key' => client_kp.address, 'weight' => 1}

],

signers: [

{'key' => client_kp.address, 'weight' => 1},

{'key' => client_kp.address, 'weight' => 1}

],

We can't pass arrays because the Contract will raise an exception if its not a Set, but I can use different weights so they're considered unique.

When this happens, verify_challenge_tx_threshold will use the weight specified in the first hash seen. Duplicate signers' weights will be ignored.

Ah, I missed that the Contract we've defined requires us to pass a Set. Got it!

leighmcculloch · 2020-03-23T20:15:38Z

spec/lib/stellar/sep10_spec.rb

+          signers: Set[
+            {'key' => client_kp.address, 'weight' => 1},
+            {'key' => client_kp.address, 'weight' => 1}
+          ],


Same here as above.

…g a Set

JakeUrban added 2 commits March 9, 2020 16:44

Added steps to development section

e554a62

Implemented SEP10 multisig functionality (no tests yet)

b2d942f

leighmcculloch self-requested a review March 10, 2020 23:38

JakeUrban added 9 commits March 10, 2020 16:40

Adjust attr_reader from keypair to address

854c514

Moved all SEP-10 related functions from Client to SEP10 class

60d68e0

Got tests passing after moving sep10 functions to new class

0ca7e73

added new tests for SEP10 multisig support

cbefc39

Added skipped test for load_account_signers

bc2e23f

Expect client master key

e89d676

Started example, setup multisig account

89902f9

Add Account.thresholds and adjust load_account_signers

43a2e8c

finish example

807f64b

msfeldstein reviewed Mar 12, 2020

View reviewed changes

msfeldstein self-requested a review March 12, 2020 20:46

msfeldstein approved these changes Mar 12, 2020

View reviewed changes

JakeUrban added 3 commits March 12, 2020 13:53

remove duplicate signers in verify_transaction_signatures

25228f6

Fix duplicate signer test

f11d90b

add .vscode to .gitignore

813a732

jimdanz reviewed Mar 13, 2020

View reviewed changes

examples/07_sep10.rb Show resolved Hide resolved

jimdanz reviewed Mar 13, 2020

View reviewed changes

spec/lib/stellar/sep10_spec.rb Outdated Show resolved Hide resolved

JakeUrban added 4 commits March 13, 2020 10:18

remove escape character in test

22f12ed

fixed load_account_signers test

c86d8a4

Add back wrappers of old client functions to preserve backwards compa…

aedac1e

…tibility

Forgot to add verify_tx_signed_by back

462b9f3

leighmcculloch suggested changes Mar 13, 2020

View reviewed changes

JakeUrban and others added 3 commits March 15, 2020 11:19

Update README.md

4c76e2f

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

Update README.md 2

2820de4

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

Update README.md 3

27d928e

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

JakeUrban added 6 commits March 16, 2020 12:30

Skip non-G accounts

3dfc3a0

Make account signers and thresholds readonly

2fe37d6

Removed AccountSigner, load_account

be09f5d

adjust example

45886db

Added deprecated comment to verify_challenge_tx

d6ce5ad

Ensure no signature can be matched more than once

92f967c

JakeUrban added 3 commits March 16, 2020 19:09

Revert "postpone refactor of client.rb sep10 functions for separate pr"

36ceed0

This reverts commit b8c8212.

Adjust parameter names for deprecated function wrappers

8b5aadb

Check for G-address properly

0f2cfaa

JakeUrban mentioned this pull request Mar 17, 2020

Discuss Stellar::Client Refactor #70

Closed

leighmcculloch reviewed Mar 19, 2020

View reviewed changes

JakeUrban and others added 9 commits March 19, 2020 11:52

Mirror Go SDK for verify_challenge_tx_signers

6f83bcd

Fixed tests, adjusted other functions

79c855a

end adjustment 1

ac1fd62

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

end adjustment 2

69b1ad2

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

end adjustment 3

283f0ad

Co-Authored-By: Leigh McCulloch <leigh@mcchouse.com>

use kp.verify instead of verify_tx_signed_by

1d252d1

added tests for read_challenge_tx and verify_challenge_tx_signers

6017063

added more tests for verify_challenge_tx_threshold

7afa35a

clean up example

4332f42

leighmcculloch reviewed Mar 21, 2020

View reviewed changes

JakeUrban added 3 commits March 23, 2020 09:51

remove signer keys from found set to avoid double counting weights

dd6e046

Add test to ensure the previous functionality remains

9a40d1b

also added a test to _threshold function for duplicate signatures

4847784

leighmcculloch approved these changes Mar 23, 2020

View reviewed changes

Changed duplicate signer weights so they aren't de-duplicated by usin…

302b297

…g a Set

JakeUrban merged commit b453494 into astroband:master Mar 23, 2020

JakeUrban deleted the sep10-multisig branch March 23, 2020 20:48

JakeUrban mentioned this pull request Mar 26, 2020

v0.8.0 #76

Merged

Conversation

JakeUrban commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeUrban commented Mar 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msfeldstein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jimdanz commented Mar 13, 2020

Uh oh!

leighmcculloch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JakeUrban commented Mar 16, 2020

Uh oh!

leighmcculloch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leighmcculloch Mar 19, 2020 • edited by JakeUrban Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeUrban Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leighmcculloch Mar 21, 2020 • edited by JakeUrban Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leighmcculloch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leighmcculloch Mar 21, 2020 • edited by JakeUrban Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leighmcculloch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeUrban commented Mar 10, 2020 •

edited

Loading

leighmcculloch Mar 19, 2020 •

edited by JakeUrban

Loading

JakeUrban Mar 19, 2020 •

edited

Loading

leighmcculloch Mar 21, 2020 •

edited by JakeUrban

Loading

leighmcculloch left a comment •

edited

Loading

leighmcculloch Mar 21, 2020 •

edited by JakeUrban

Loading