Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
xdr, keypair: Add helpers to create CAP-40 decorated signatures #4302
xdr, keypair: Add helpers to create CAP-40 decorated signatures #4302
Changes from 4 commits
fd6a1c2
1b155d1
69b50bd
f63acb7
fb2a5fa
f89c9d7
0498dc6
d37806d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
❗ The first
t.Run
block is run twice with the same inputs, but then asserts on separate rows in the table that just happened to be constant:As mentioned before, there's unnecessary uncoupling testing these two functions which is resulting in a set of tests that are not really clear. Can you split them up?
For example, if you remove the table test framework that is driven by a mixture of shared inputs and unique inputs, you end up with the following that while is less clever, is approximately the same amount of code:
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.
Ah, I see what you're getting at. Wdyt about d37806d? Keeps the extensible table of tests (and adds a length-zero test), but completely separates it from the
NewDecoratedSignature
test, and minimizes the scattered "magic byte arrays".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 recommend viewing the whole file rather than the commit diff.)