feat: Unified facades for the Wallet Lib#100
Conversation
| We should consider keeping a `v2` version with minor and patch updates for | ||
| critical issues, but going forward we would fully migrate to `v3`. |
There was a problem hiding this comment.
No need to keep a v2 going if we release a v3
There was a problem hiding this comment.
Removed the mention of keeping the v2 on 4e9f712
| Why are we doing this? What use cases does it support? What is the expected | ||
| outcome? |
| # Rationale and alternatives | ||
| [rationale-and-alternatives]: #rationale-and-alternatives | ||
|
|
||
| The facades could be kept separate and a wrapper `WalletWrapper` could be |
There was a problem hiding this comment.
Since we are introducing a new major version we can add more breaking changes, maybe even change how we think of facades to make managing the connections better, what do you think?
There was a problem hiding this comment.
It's a good idea! My main question is how to organize the rollout of those breaking changes.
The first thing to come to mind is to have a beta branch where we could test only in our own clients ( Desktop/Mobile/Headless/Web Wallets ) in increasingly breaking changes with no major version upgrades for a few months.
After the changes stabilize, we make the stable version available for external clients as well.
Do you have other ideas on how to make those multiple changes gradually and incrementally?
There was a problem hiding this comment.
Added a section to discuss a beta branch on 055f648 .
There was a problem hiding this comment.
Added the suggestion of re-think the facades as a Future Improvement on 4e9f712
andreabadesso
left a comment
andreabadesso
left a comment
There was a problem hiding this comment.
I don't disagree with anything on the design, but I miss an analysis of impact on all of our wallet-lib clients that use the wallet-service -- wallet-mobile, wallet-desktop, wallet-headless, wallet-service, etc...
|
All client wallets will have to adapt to the breaking changes proposed here and implemented in the When we close the beta version we should have a very clear guide on how any external client could adapt to those changes. |
tuliomir
changed the title
pedroferreira1
left a comment
pedroferreira1
left a comment
There was a problem hiding this comment.
For me this is approved and it's important to be done. It shouldn't be complicated to execute the most important part of this design.
- 3 methods have inconsistent async/sync signatures (critical)
- Return types differ significantly for several shared methods
- The IHathorWallet interface has incomplete type annotations
These are the most important ones, they must be addressed in different PRs and then we must release a new version of the wallet lib. It shouldn't take much time to fix these in the clients.
- 4 methods throw "Not implemented" in WalletServiceWallet
- ~20 methods exist only in HathorWallet
I don't think this should be addressed now. This difference may be for many reasons, and most of them is that we didn't want to make this development a priority back then. Only the HathorWallet facade is used by the headless wallet, which has support for multisig sending, tx template, and other features that we don't have in the other wallets. We don't want to add this support right now, so I believe we shouldn't consider this a part of this project. In the tests, we can have tests for the whole interface and some specific tests for methods that are specific of the HathorWallet class.
What do you think?
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
4 participants
The Wallet Lib facades for the Fullnode and the Wallet Service should share a subset of the most important methods, with the same parameters and output data format.
This will cause breaking changes that will require a major version update on the lib.
See rendered.