-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Unified facades for the Wallet Lib #100
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
Changes from 2 commits
4917622
64d947e
5bb3d2c
055f648
de4018b
8d7f3b0
4e9f712
a02fef7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,306 @@ | ||
| - Feature Name: wallet-lib-shared-facades | ||
| - Start Date: 2026-01-21 | ||
| - RFC PR: (leave this empty) | ||
| - Hathor Issue: (leave this empty) | ||
| - Author: tulio@hathor.network | ||
|
|
||
| # Summary | ||
| [summary]: #summary | ||
|
|
||
| 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. | ||
|
|
||
| # Motivation | ||
| [motivation]: #motivation | ||
|
|
||
| Why are we doing this? What use cases does it support? What is the expected | ||
| outcome? | ||
|
|
||
| At the moment, both facades are similar, but not equal. This causes duplicated | ||
| test suites, requires additional time and attention from the developers and | ||
| ultimately introduces bugs on the Wallet Lib clients when they need to switch | ||
| between facades. | ||
|
|
||
| # Guide-level explanation | ||
| [guide-level-explanation]: #guide-level-explanation | ||
|
|
||
| To achieve this state it's necessary to change the contracts for some methods, | ||
| both in the wallets and in their shared interface `IHathorWallet`, which is now | ||
| outdated. An [initial investigation](./shared-contract-investigation.md) was made | ||
| to understand the necessary changes, and they will be summarized below. | ||
|
|
||
| - Change methods from `sync` to `async` on the Wallet Service facade | ||
| - Modify `IHathorWallet` return types that currently do not match the fullnode facade | ||
| - Create types for the parameters of both facades, facilitating visual inspection from devs | ||
| - Decide on `UTXO` and `Tx History` objects that currently differ, and are used by most of the applications | ||
| - Decide which subset of methods will consist of the `IHathorWallet`, and leave each facade to have its own complementary methods | ||
| - Add methods that are present in both facades but not in the `IHathorWallet`, such as `getAuthorityUtxo()` | ||
| - Create a `shared_types.ts` on the root folder, instead of using types from each facade folder | ||
|
|
||
| At the end of this project, it should be possible for a hypothetical client | ||
| implementation to call methods from a Fullnode or Wallet Service facades | ||
| interchangeably during its operation, with no adaptation to their inner workings. | ||
|
|
||
| # Reference-level explanation | ||
| [reference-level-explanation]: #reference-level-explanation | ||
|
|
||
| ## 1. Async/Sync Signature Alignment | ||
|
|
||
| The following methods must become async in both facades. The Wallet Service facade | ||
| currently returns synchronous values, while the Fullnode facade returns Promises. | ||
|
|
||
| ### 1.1 Address Methods | ||
|
|
||
| ```typescript | ||
| // Before (WalletServiceWallet - sync) | ||
| getCurrentAddress(options?: { markAsUsed: boolean }): AddressInfoObject; | ||
| getNextAddress(): AddressInfoObject; | ||
|
|
||
| // After (both facades - async) | ||
| getCurrentAddress(options?: { markAsUsed: boolean }): Promise<AddressInfoObject>; | ||
| getNextAddress(): Promise<AddressInfoObject>; | ||
| ``` | ||
|
|
||
| ### 1.2 Lifecycle Methods | ||
|
|
||
| ```typescript | ||
| // Before (IHathorWallet - sync) | ||
| stop(): void; | ||
|
|
||
| // After (IHathorWallet - async, matching both implementations) | ||
| stop(): Promise<void>; | ||
| ``` | ||
|
|
||
| ## 2. Unified Return Types | ||
|
|
||
| ### 2.1 Address Info Object | ||
|
|
||
| Both facades will return a consistent `AddressInfoObject`: | ||
|
|
||
| ```typescript | ||
| interface AddressInfoObject { | ||
| address: string; | ||
| index: number; | ||
| addressPath: string; | ||
| info?: string; // Optional, WalletService-specific metadata | ||
| } | ||
| ``` | ||
|
|
||
| **Corner case:** `index` is `number | null` in Fullnode facade. This will be changed to | ||
| always `number`, as unknown/unindexed addresses are not possible in the scope of this method. | ||
|
|
||
| ### 2.2 Authority UTXO Type | ||
|
|
||
| Create a unified `AuthorityUtxo` type for `getMintAuthority()`, `getMeltAuthority()`, | ||
| and `getAuthorityUtxo()`: | ||
|
|
||
| ```typescript | ||
| interface AuthorityUtxo { | ||
| txId: string; | ||
| index: number; | ||
| address: string; | ||
| authorities: OutputValueType; | ||
| // Optional fields present when available from storage | ||
| tokenId?: string; | ||
| value?: OutputValueType; | ||
| timelock?: number | null; | ||
| } | ||
| ``` | ||
|
|
||
| Both facades will return `Promise<AuthorityUtxo[]>`. The Fullnode facade currently | ||
| returns `IUtxo[]` (richer), while WalletService returns `AuthorityTxOutput[]` (minimal). | ||
| The unified type uses the intersection of required fields, with optional enrichment. | ||
|
|
||
| ### 2.3 Transaction Return Types | ||
|
|
||
| ```typescript | ||
| // Standardize to non-nullable return | ||
| sendTransaction(...): Promise<Transaction | null>; | ||
| sendManyOutputsTransaction(...): Promise<Transaction | null>; | ||
| ``` | ||
|
|
||
| **Corner case:** Fullnode facade returns `null` when `startMiningTx: false`. This | ||
| null possibility will be enforced in the other facade as well. | ||
|
|
||
| ## 3. Interface Method Additions | ||
|
|
||
| Add methods that exist in both facades but are missing from `IHathorWallet`: | ||
|
|
||
| ```typescript | ||
| interface IHathorWallet { | ||
|
|
||
| // Authority methods (currently missing) | ||
| getMintAuthority(tokenUid: string, options?: AuthorityOptions): Promise<AuthorityUtxo[]>; | ||
| getMeltAuthority(tokenUid: string, options?: AuthorityOptions): Promise<AuthorityUtxo[]>; | ||
| getAuthorityUtxo(tokenUid: string, authority: 'mint' | 'melt', options?: AuthorityOptions): Promise<AuthorityUtxo[]>; | ||
|
|
||
| // State methods (currently missing) | ||
| isReady(): boolean; | ||
|
|
||
| // Cleanup methods (currently missing) | ||
| clearSensitiveData(): void; | ||
| isHardwareWallet(): boolean; | ||
| } | ||
|
|
||
| // Will be declared on the dedicated shared types file | ||
| interface AuthorityOptions { | ||
| many?: boolean; | ||
| skipSpent?: boolean; // Replaces only_available_utxos | ||
| filterAddress?: string; // Replaces filter_address (camelCase) | ||
| } | ||
| ``` | ||
|
|
||
| ## 4. Typed Options Parameters | ||
|
|
||
| Replace untyped `options` with explicit types: | ||
|
|
||
| ```typescript | ||
| interface CreateTokenOptions { | ||
| address?: string; | ||
| changeAddress?: string; | ||
| createMint?: boolean; | ||
| mintAuthorityAddress?: string; | ||
| allowExternalMintAuthorityAddress?: boolean; | ||
| createMelt?: boolean; | ||
| meltAuthorityAddress?: string; | ||
| allowExternalMeltAuthorityAddress?: boolean; | ||
| data?: string; | ||
| pinCode?: string; | ||
| } | ||
|
|
||
| interface SendTransactionOptions { | ||
| token?: string; | ||
| changeAddress?: string; | ||
| pinCode?: string; | ||
| } | ||
|
|
||
| interface SendManyOutputsOptions { | ||
| inputs?: Array<{ txId: string; index: number }>; | ||
| changeAddress?: string; | ||
| pinCode?: string; | ||
| } | ||
| ``` | ||
|
|
||
| ## 5. Shared Types File Structure | ||
|
|
||
| Create `src/shared_types.ts` containing: | ||
|
|
||
| ``` | ||
| src/ | ||
| ├── shared_types.ts # New: All shared interface types | ||
| ├── new/ | ||
| │ └── wallet.ts # HathorWallet (Fullnode facade) | ||
| │ └── types.ts # Fullnode-specific types only | ||
| └── wallet/ | ||
| └── wallet.ts # HathorWalletServiceWallet | ||
| └── types.ts # WalletService-specific types only | ||
| ``` | ||
|
|
||
| The `IHathorWallet` interface moves from `src/wallet/types.ts` to `src/shared_types.ts`, | ||
| along with all types used in interface method signatures. | ||
|
|
||
| ## 6. Methods Remaining Facade-Specific | ||
|
|
||
| The following methods will NOT be added to `IHathorWallet` and remain facade-specific: | ||
|
|
||
| **Fullnode-only:** | ||
| - `buildTxTemplate()`, `runTxTemplate()` - Transaction template system | ||
| - `getFullHistory()` - Large dataset, different storage models | ||
| - `consolidateUtxos()` - Requires local UTXO management | ||
| - `setGapLimit()`, `getGapLimit()` - Local address derivation control | ||
| - `syncHistory()`, `reloadStorage()` - Local storage operations | ||
| - Multisig methods - Different signing workflows | ||
|
|
||
| **WalletService-only:** | ||
| - Service-specific connection management | ||
| - Polling configuration | ||
|
|
||
| ## 7. Implementation Order | ||
|
|
||
| 1. Create `shared_types.ts` with unified types | ||
| 2. Update `IHathorWallet` interface with new signatures | ||
| 3. Modify WalletServiceWallet sync methods to async | ||
| 4. Update return types in both facades to match interface | ||
| 5. Add missing methods to interface | ||
| 6. Update existing tests to use shared test patterns | ||
| 7. Create shared test suite for interface compliance | ||
|
|
||
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
|
||
| The only reasons not to do this would be to preserve backwards compatibility with | ||
| existing clients, or to dedicate development priority elsewhere. | ||
|
|
||
| # Rationale and alternatives | ||
| [rationale-and-alternatives]: #rationale-and-alternatives | ||
|
|
||
| The facades could be kept separate and a wrapper `WalletWrapper` could be | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a section to discuss a beta branch on 055f648 .
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the suggestion of re-think the facades as a Future Improvement on 4e9f712 |
||
| developed to abstract away their differences. This would allow for shared | ||
| integration tests and a simpler developer experience, while the internal work | ||
| is postponed for a better time. | ||
|
|
||
| The downside of this alternative is having additional work and complexity in | ||
| order to avoid a synchronization work that will have to be done eventually. | ||
|
|
||
| By not implementing this shared facade, we keep the responsibility on the client | ||
| to know what facade is being used and the details of each one separately in the | ||
| client code. | ||
|
|
||
| # Prior art | ||
| [prior-art]: #prior-art | ||
|
|
||
| The objetive of the `IHathorWallet` interface was to create a shared contract for | ||
| both facades, but without automated testing it was natural to drift away from | ||
| a compliance strict enough for a full Typescript validation. | ||
|
|
||
| The main proposal of this RFC is to bring this interface up-to-date with the | ||
| current state of the wallets, improving it albeit with breaking changes. | ||
|
|
||
| # Unresolved questions | ||
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| The main questions to resolve through this RFC are the actual method signatures, | ||
| deciding which properties are mandatory, which will be optional. | ||
|
|
||
| A special attention should be paid to the `null` parameter, which is used in | ||
| many places as a valid replace for `undefined`. This is | ||
|
|
||
| We should consider keeping a `v2` version with minor and patch updates for | ||
| critical issues, but going forward we would fully migrate to `v3`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to keep a v2 going if we release a v3
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the mention of keeping the v2 on 4e9f712 |
||
|
|
||
| - What parts of the design do you expect to resolve through the RFC process | ||
| before this gets merged? | ||
| - What parts of the design do you expect to resolve through the implementation | ||
| of this feature before stabilization? | ||
| - What related issues do you consider out of scope for this RFC that could be | ||
| addressed in the future independently of the solution that comes out of this | ||
| RFC? | ||
|
|
||
| # Future possibilities | ||
| [future-possibilities]: #future-possibilities | ||
|
|
||
| Think about breaking the facade files into multiple pieces with common | ||
| group responsabilities, that could be shared between the two facades. This would | ||
| make it easier for LLMs to interact with those files, instead of having | ||
| two facades with over 3k lines that do not fit their context windows. | ||
|
|
||
| Think about what the natural extension and evolution of your proposal would be | ||
| and how it would affect the network and project as a whole in a holistic way. | ||
| Try to use this section as a tool to more fully consider all possible | ||
| interactions with the project and network in your proposal. Also consider how | ||
| this all fits into the roadmap for the project and of the relevant sub-team. | ||
|
|
||
| This is also a good place to "dump ideas", if they are out of scope for the | ||
| RFC you are writing but otherwise related. | ||
|
|
||
| If you have tried and cannot think of any future possibilities, | ||
| you may simply state that you cannot think of anything. | ||
|
|
||
| Note that having something written down in the future-possibilities section | ||
| is not a reason to accept the current or a future RFC; such notes should be | ||
| in the section on motivation or rationale in this or subsequent RFCs. | ||
| The section merely provides additional information. | ||
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.
remove?
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.
Thanks, removed on 5bb3d2c