RE: [beem] one-time private key / wallet compatibility
Hi @blockchainstudio, thank you for your contribution.
You made a great post, again.
Although I never used Beempy, the contribution is clear and understandable.
If you check the answers of the questionnaire you could say that this one is not fair because you added a lot of details and you also created an Issue and a PR on GitHub.
Has the contributor proposed a possible solution to implement the suggestion?
Yes, but the possible solution was not described in sufficient detail.
I chose that because crokkon and holger told me that:
The implementation (transactionbuilder.py) can potentially break a lot of other things. Added this to the library itself (transactionbuilder.py) is not a good idea.
Also, holger told me that:
It is technically possible to improve beempy, so that it asks the active key when it is not in the wallet.
I think it is a good idea and I will try add it in my next release
Your contribution has been evaluated according to Utopian policies and guidelines, as well as a predefined set of questions pertaining to the category.
To view those questions and the relevant answers related to your post, click here.
Need help? Chat with us on Discord.
Hi @favcau, thank you so much for your very detailed feedback and I'm happy with the score. So don't get me wrong and I just like to add some comments.
I agree in general with @crokkon that "added this to the library itself (transactionbuilder.py) is not a good idea.", since that file is a sort-of base library (shared with steem-python), so it might be better to be untouched although that's why I also submitted my PR for the official library.
But I actually don't agree with "The implementation (transactionbuilder.py) can potentially break a lot of other things." :) If you can read the code, you can see that it's actually safe, and if it's unsafe, then the existing and related codes have the serious problem. Even if a wrong private key is provided somehow, the error should be handled properly, and of course I've also already checked that.
While I also agree with that it's better to change more high-level files to add this feature, I chose a more easier (but again still safe and legit) way as an external contributor. One lesson I learned from other contributions are if a PR is complicated, then it tends to be ignored :) So that's why I always prefer a very simple fix so that PO can easily know that it's working without a problem :)
But @holger80 is quite active in dev, so maybe more complicated one might have been okay :) I'll change the PR or just close it depending on his response.
Many thanks everybody!
Hi @blockchainstudio, beem is primarily a library where other tools build upon. beempy is one example but by far not the only one. Having
transactionbuildercallprintor asking for a key viagetpassis no problem for beempy since it is a CLI anyway. However for non-CLI applications, bots, cronjobs, this would unexpectedly print things to stdout and block on getpass instead of raising an exception that can be handled on the higher-level code. beem/cli.py is IMO the right place to implement this great suggestion.Hi @crokkon, thank you so much for your detailed comment and now I understand what you meant and fully agree with you! I misunderstood your comment like you meant the code itself may not work :)
One last comment is of course I also tried to change cli.py and thought it was the right place to change, but you know in that case, I basically need to change every command that may raise the exception, which was too much, so I took the bad design without thinking further :) But you and @holger80 are 100% right in this case. Thank you so much again and I'll close my PR.
Are you planing to make a PR in which cli.py is adapted? Please let me know, so that we both do not work for the same changes..
Thank you for contacting me. It seems you already have interest in doing it, then I think it's better for you to do it for efficiency (unless you want me to do it). What about the wallet compatibility? Are you interested in that too?
Thank you for your review, @favcau! Keep up the good work!