-
Notifications
You must be signed in to change notification settings - Fork 94
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
Support Segwit addresses in orderbooks #1064
Conversation
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.
Great work 🔥, I have only 2 minor comments 🙂
@shamardy I've pushed the fixes, please review again 🙂 |
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 did not understand all the notions of Rust that I saw in the pull request, but what I understood and read seems correct to me!
One small question regarding: https://github.com/KomodoPlatform/atomicDEX-API/pull/1064/files#diff-419a1934bf5c18ab0befab6e49dbf0d084e992cce06aba1b033ddef608889101R22
In general, for such constants, do we want to have a commonplace to facilitate the writing of unit tests?
I see, for example, that in https://github.com/KomodoPlatform/atomicDEX-API/pull/1064/files#diff-6d3ec491778e38704498865bb58b487ca0abfe56dd4bf794c6579c5fdadda313R8058 we use the same content.
@Milerius Yes, it can be useful in the future. But it will take a bit more time to refactor, which is out of this task context. I did it because I had few tests failing due to the unavailability of some electrums - they used only one or two servers. |
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.
LGTM!
#826
Also related to #1046. It's the second try to implement a backward-compatible way of displaying segwit addresses in the order books 🙂
Thanks to @shamardy and @sergeyboyko0791 for brainstorming on this matter!