Skip to content
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

Add SEP-5 tests for Shamir, more documentation and examples #1015

Merged
merged 10 commits into from
Jan 11, 2025

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Jan 3, 2025

Follow up of #1012

(Note I will link the PR to the SEP-5 itself once I put it up.)

EDIT this is the update to the SEP stellar/stellar-protocol#1601

Copy link
Member

@overcat overcat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Only a few details may be worth revising, and I have placed them in the comments.

stellar_sdk/keypair.py Show resolved Hide resolved
stellar_sdk/keypair.py Outdated Show resolved Hide resolved
docs/en/generate_keypair.rst Show resolved Hide resolved
stellar_sdk/keypair.py Show resolved Hide resolved
stellar_sdk/keypair.py Outdated Show resolved Hide resolved
stellar_sdk/keypair.py Show resolved Hide resolved
stellar_sdk/keypair.py Show resolved Hide resolved
stellar_sdk/keypair.py Show resolved Hide resolved
Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.20%. Comparing base (4200224) to head (99ea57f).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1015   +/-   ##
=======================================
  Coverage   98.20%   98.20%           
=======================================
  Files         261      261           
  Lines       14066    14075    +9     
=======================================
+ Hits        13813    13822    +9     
  Misses        253      253           
Flag Coverage Δ
unittests 98.20% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
stellar_sdk/keypair.py 98.98% <ø> (ø)
stellar_sdk/sep/mnemonic.py 100.00% <ø> (ø)
tests/test_keypair.py 100.00% <100.00%> (ø)

Copy link
Contributor Author

@tupui tupui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusting for entropy instead of BIP-39

docs/en/generate_keypair.rst Outdated Show resolved Hide resolved
docs/en/generate_keypair.rst Outdated Show resolved Hide resolved
stellar_sdk/keypair.py Outdated Show resolved Hide resolved
stellar_sdk/keypair.py Outdated Show resolved Hide resolved
stellar_sdk/keypair.py Outdated Show resolved Hide resolved
tests/test_keypair.py Outdated Show resolved Hide resolved
Copy link
Member

@overcat overcat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this excellent PR! The code is clean and well-documented.
But I noticed that one test case failed; can you update it? Once the tests pass, we can merge it.

@tupui
Copy link
Contributor Author

tupui commented Jan 10, 2025

Done 😃

@tupui
Copy link
Contributor Author

tupui commented Jan 10, 2025

Oh wait there is something fishy.

@tupui
Copy link
Contributor Author

tupui commented Jan 10, 2025

ok I had to revert the to_entropy in the tests. See the comment in the other PR too. Depending on what you say there, I can adjust this with separate keys so we use entropy still.

Copy link
Member

@overcat overcat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@overcat overcat merged commit f320274 into StellarCN:main Jan 11, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants