trezor / python-trezor

:snake: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU Lesser General Public License v3.0
201 stars 194 forks source link

Stellar: _xdr_read_address now returns a string #281

Closed zulucrypto closed 6 years ago

zulucrypto commented 6 years ago

Another minor change I found to make things compatible with the new message format that sends accounts as strings.

Also includes a bonus test for setting the inflation destination.

zulucrypto commented 6 years ago

@tsusanka - FYI I targeted this against your branch since it looked like it was still under development, let me know if it should be merged against a different one.

tsusanka commented 6 years ago

This is great! The commit https://github.com/trezor/python-trezor/commit/10bbb57c865ed514a01daa7b4e821207b62c7f89 got already merged to master though (from trezor:tsusanka/stellar-accounts-addresses), I think I just forgot to delete the branch, sorry about that.

Could you rebase on master and target against that?

zulucrypto commented 6 years ago

Rebased and target updated

matejcik commented 6 years ago

ah, so this is basically a bugfix for trezorctl -- otherwise parsing transactions would fail, right?

Would you mind adding a test for parse_transaction_bytes? Probably to tests/unit_tests/stellar.py, with a couple test vectors and assertions that the resulting protobuf objects are the right ones?

zulucrypto commented 6 years ago

@matejcik - Added unit tests for each operation (and caught a few more bugs in the process!)

matejcik commented 6 years ago

@zulucrypto awesome, glad to see bugs being caught .)

there's one more bug that the test revealed to me: address_from_public_key returns bytes -- and you're testing for bytes in the tests. These should be strings, per PR title :)

this is going to be caught by type checking in the future, hopefully

zulucrypto commented 6 years ago
matejcik commented 6 years ago

perfect, thank you. merged via b2f35de8b8544c15c07503251e88b1eaa4f8703f