Refactor: Fullnode facade to Typescript#972
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #972 +/- ##
==========================================
- Coverage 84.75% 84.66% -0.10%
==========================================
Files 112 112
Lines 8146 8228 +82
Branches 1753 1787 +34
==========================================
+ Hits 6904 6966 +62
- Misses 1214 1233 +19
- Partials 28 29 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7750435 to
427ef03
Compare
There was a problem hiding this comment.
Pull request overview
This PR initiates the TypeScript migration of the HathorWallet facade (src/new/wallet.ts) by renaming it from .js to .ts and typing all variables, parameters, and return values as any. The migration includes adding ESLint suppressions and TypeScript's @ts-nocheck directive to allow gradual migration without breaking the build.
Key Changes:
- Renamed
src/new/wallet.jstosrc/new/wallet.tswith all types set toany - Added property declarations for
storageandloggerfields - Updated interface return types to support both wallet facades
- Added stub implementations for
startReadOnly()andgetReadOnlyAuthToken()methods - Enhanced
NanoContractTransactionBuilderwith null checks viaassertWallet()method
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/new/wallet.ts | Migrated from JavaScript to TypeScript with any types throughout, added property declarations, and stub method implementations |
| src/new/sendTransaction.ts | Changed wallet property type to HathorWallet | null to accommodate both facades |
| src/wallet/types.ts | Updated interface return types with union types to accommodate differences between wallet facades |
| src/wallet/wallet.ts | Added HathorWallet import and unsafe type casts for NanoContractTransactionBuilder compatibility |
| src/nano_contracts/builder.ts | Added assertWallet() guard method to ensure wallet is set before usage |
| jest-integration.config.js | Updated file path reference from .js to .ts |
Comments suppressed due to low confidence (4)
src/new/wallet.ts:123
- The word "proprieties" should be "properties" in this comment.
src/new/wallet.ts:11 - The ESLint directive structure is confusing and potentially ineffective. Lines 8-11 disable the
ban-ts-commentrule, enable@ts-nocheck, then re-enable theban-ts-commentrule. However, this is problematic because:
- The
eslint-enableon line 11 doesn't affect the@ts-nocheckon line 10 (which is a TypeScript directive, not an ESLint rule) - The
ban-ts-commentrule is typically used to prevent@ts-ignoreand similar directives, but here it's being temporarily disabled just to use@ts-nocheck
Consider simplifying this to just:
/* eslint-disable @typescript-eslint/no-explicit-any */
// @ts-nocheckOr better yet, as mentioned in another comment, consider removing @ts-nocheck entirely to get actual TypeScript checking benefits.
src/new/wallet.ts:3505
- The stub implementations of
startReadOnly()andgetReadOnlyAuthToken()throw genericErrorobjects with inconsistent capitalization in the error messages ("Not Implemented" vs "Not implemented."). These should throw a more specific error type likeNotImplementedErroror at least use consistent error messages. Additionally, consider documenting why these methods are not implemented in this facade.
src/new/wallet.ts:10 - The
@ts-nocheckdirective on line 10 disables all TypeScript checking for this file, which defeats the purpose of migrating to TypeScript. While the PR description mentions this is for gradual migration, having@ts-nocheckmeans TypeScript provides no benefits at all - not even basic type checking.
Consider using more targeted suppression like @ts-expect-error on specific problematic lines, or removing this directive and fixing the issues incrementally. This would provide immediate value from the TypeScript migration by catching type errors while still allowing the use of any types.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * */ | ||
| export default class SendTransaction extends EventEmitter { | ||
| wallet: HathorWallet; | ||
| wallet: HathorWallet | null; |
There was a problem hiding this comment.
Changing wallet: HathorWallet to wallet: HathorWallet | null alters the existing API contract. This could introduce potential null reference errors in code that depends on SendTransaction. Since this class is used by both wallet facades, consider whether this change is necessary or if there are other locations in the code that need to be updated to handle the null case safely.
If the wallet can truly be null, ensure all usages of this.wallet in the SendTransaction class properly handle the null case with guards.
There was a problem hiding this comment.
This is already ensured, the only change was to make the type declaration comply with the code.
There are code branches that expect this.wallet not to be present.
| getCurrentAddress(options?: { markAsUsed: boolean }): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type | ||
| getNextAddress(): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
These return type changes with | Promise<unknown> are inconsistent with the actual implementations. The HathorWalletServiceWallet returns Promise<AddressInfoObject> while HathorWallet appears to return AddressInfoObject directly based on the implementation. However, looking at the new wallet.ts file, getCurrentAddress and getNextAddress both return Promise<any>, which means they're actually async and return Promises.
The issue is that the comment says "FIXME: Should have a single return type" but the union type still includes both synchronous and asynchronous return types. If both implementations are actually async (as they appear to be in the migrated code), the return type should just be Promise<AddressInfoObject> without the AddressInfoObject | part.
| getCurrentAddress(options?: { markAsUsed: boolean }): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type | |
| getNextAddress(): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type; | |
| getCurrentAddress(options?: { markAsUsed: boolean }): Promise<AddressInfoObject>; | |
| getNextAddress(): Promise<AddressInfoObject>; |
There was a problem hiding this comment.
This is one of the mapped issues: the Wallet Service facade returns synchronously and the fullnode facade returns asynchronously.
Fixing it is outside the scope of this PR, we're only mapping and highlighting an existing issue.
| options: DestroyAuthorityOptions | ||
| ): Promise<Transaction>; | ||
| getFullHistory(): TransactionFullObject[]; | ||
| getFullHistory(): TransactionFullObject[] | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
Similar to getCurrentAddress and getNextAddress, this union type suggests inconsistent implementations. If both facades actually return Promise<unknown>, then the synchronous TransactionFullObject[] part should be removed. The FIXME comment acknowledges this needs fixing, but the actual implementation in the migrated code appears to be async (line 1316 shows async getFullHistory(): Promise<any>), so the return type should just be Promise<TransactionFullObject[]> (or Promise<{[txId: string]: TransactionFullObject}> based on the implementation which returns a keyed object, not an array).
| getFullHistory(): TransactionFullObject[] | Promise<unknown>; // FIXME: Should have a single return type; | |
| getFullHistory(): Promise<{[txId: string]: TransactionFullObject}>; // Fixed: Single async return type |
There was a problem hiding this comment.
Same issue here: the facades have different types of execution. Mapping and highlighting for now.
src/wallet/wallet.ts
Outdated
| CreateNanoTxData, | ||
| } from '../nano_contracts/types'; | ||
| import { WalletServiceStorageProxy } from './walletServiceStorageProxy'; | ||
| import { HathorWallet } from '../index'; |
There was a problem hiding this comment.
Importing HathorWallet from the index creates a circular dependency concern. The src/wallet/wallet.ts file imports from src/index which exports from src/lib.ts which in turn imports from both src/wallet/wallet.ts (HathorWalletServiceWallet) and src/new/wallet.ts (HathorWallet).
This import should be changed to import directly from '../new/wallet' to avoid the circular dependency through the index file.
| import { HathorWallet } from '../index'; | |
| import { HathorWallet } from '../new/wallet'; |
| const builder = new NanoContractTransactionBuilder() | ||
| .setMethod(method) | ||
| .setWallet(this) | ||
| .setWallet(this as unknown as HathorWallet) // FIXME: Should accept IHathorWallet instead |
There was a problem hiding this comment.
This cast through unknown is a code smell indicating a type system issue. The NanoContractTransactionBuilder.setWallet() method expects a HathorWallet instance, but HathorWalletServiceWallet implements IHathorWallet and is not a HathorWallet.
The proper fix would be to make NanoContractTransactionBuilder.setWallet() accept IHathorWallet instead of the concrete HathorWallet class. This would eliminate the need for unsafe type casts and make the builder work with both wallet implementations without forcing a type lie.
| .setWallet(this as unknown as HathorWallet) // FIXME: Should accept IHathorWallet instead | |
| .setWallet(this) |
There was a problem hiding this comment.
That's precisely what the comment is suggesting. The issue is mapped and highlighted.
| */ | ||
| private assertWallet(): asserts this is { wallet: HathorWallet } { | ||
| if (!this.wallet) { | ||
| throw new TypeError('Wallet is required to build nano contract transactions.'); |
There was a problem hiding this comment.
It was the closest I could get to the type of the error. Since this can be only HathorWallet or null , I decided for a type error.
Do you think a simple Error would be more adequate here?
There was a problem hiding this comment.
We could create a custom error or use an existing one.
WalletTypeError or WalletError for instance.
| getCurrentAddress(options?: { markAsUsed: boolean }): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type | ||
| getNextAddress(): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
Both of these have the return type Promise<AddressInfoObject>
There was a problem hiding this comment.
If you look at the Wallet Service facade, its response is synchronous. I could have added AddressInfoObject | Promise<AddressInfoObject> , but I decided on adhering to the principle of keeping this PR as simple as it could be. I can't use any here, so I used unknown instead.
See the synchronous code below:
hathor-wallet-lib/src/wallet/wallet.ts
Lines 1446 to 1464 in eeb9655
There was a problem hiding this comment.
It should be async but we can change that later
| options: DestroyAuthorityOptions | ||
| ): Promise<Transaction>; | ||
| getFullHistory(): TransactionFullObject[]; | ||
| getFullHistory(): TransactionFullObject[] | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
This should be Promise<TransactionFullObject[]> on both, right?
There was a problem hiding this comment.
Yes, it should, but for now it isn't.
See below:
hathor-wallet-lib/src/wallet/wallet.ts
Lines 1625 to 1627 in eeb9655
There was a problem hiding this comment.
Yeah, this one is not implemented.
We can change the return type without consequences
| const builder = new NanoContractTransactionBuilder() | ||
| .setMethod(method) | ||
| .setWallet(this) | ||
| .setWallet(this as unknown as HathorWallet) // FIXME: Should accept IHathorWallet instead |
There was a problem hiding this comment.
Yes, with the following error:
TS2345: Argument of type this is not assignable to parameter of type HathorWallet
Type HathorWalletServiceWallet is missing the following properties from type HathorWallet:
logger, setGapLimit, indexLimitLoadMore, indexLimitSetEndIndex
, and 38 more.
tuliomir
left a comment
There was a problem hiding this comment.
GitHub is forcing me to add a review so that my responses to comments are placed here.
| async executeDeposit( | ||
| action: NanoContractAction | ||
| ): Promise<{ inputs: IDataInput[]; outputs: IDataOutput[] }> { | ||
| this.assertWallet(); |
There was a problem hiding this comment.
Most methods in this file assume this.wallet is present, but never validates it, so it was breaking the build with errors.
The use of this assert method protects and validates the type, however the test adjusments are outside the scope of this PR.
| * */ | ||
| export default class SendTransaction extends EventEmitter { | ||
| wallet: HathorWallet; | ||
| wallet: HathorWallet | null; |
There was a problem hiding this comment.
This is already ensured, the only change was to make the type declaration comply with the code.
There are code branches that expect this.wallet not to be present.
| getCurrentAddress(options?: { markAsUsed: boolean }): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type | ||
| getNextAddress(): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
This is one of the mapped issues: the Wallet Service facade returns synchronously and the fullnode facade returns asynchronously.
Fixing it is outside the scope of this PR, we're only mapping and highlighting an existing issue.
| options: DestroyAuthorityOptions | ||
| ): Promise<Transaction>; | ||
| getFullHistory(): TransactionFullObject[]; | ||
| getFullHistory(): TransactionFullObject[] | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
Same issue here: the facades have different types of execution. Mapping and highlighting for now.
src/wallet/wallet.ts
Outdated
| CreateNanoTxData, | ||
| } from '../nano_contracts/types'; | ||
| import { WalletServiceStorageProxy } from './walletServiceStorageProxy'; | ||
| import { HathorWallet } from '../index'; |
| const builder = new NanoContractTransactionBuilder() | ||
| .setMethod(method) | ||
| .setWallet(this) | ||
| .setWallet(this as unknown as HathorWallet) // FIXME: Should accept IHathorWallet instead |
There was a problem hiding this comment.
That's precisely what the comment is suggesting. The issue is mapped and highlighted.
| const builder = new NanoContractTransactionBuilder() | ||
| .setMethod(method) | ||
| .setWallet(this) | ||
| .setWallet(this as unknown as HathorWallet) // FIXME: Should accept IHathorWallet instead |
There was a problem hiding this comment.
Yes, with the following error:
TS2345: Argument of type this is not assignable to parameter of type HathorWallet
Type HathorWalletServiceWallet is missing the following properties from type HathorWallet:
logger, setGapLimit, indexLimitLoadMore, indexLimitSetEndIndex
, and 38 more.
| getCurrentAddress(options?: { markAsUsed: boolean }): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type | ||
| getNextAddress(): AddressInfoObject | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
If you look at the Wallet Service facade, its response is synchronous. I could have added AddressInfoObject | Promise<AddressInfoObject> , but I decided on adhering to the principle of keeping this PR as simple as it could be. I can't use any here, so I used unknown instead.
See the synchronous code below:
hathor-wallet-lib/src/wallet/wallet.ts
Lines 1446 to 1464 in eeb9655
| options: DestroyAuthorityOptions | ||
| ): Promise<Transaction>; | ||
| getFullHistory(): TransactionFullObject[]; | ||
| getFullHistory(): TransactionFullObject[] | Promise<unknown>; // FIXME: Should have a single return type; |
There was a problem hiding this comment.
Yes, it should, but for now it isn't.
See below:
hathor-wallet-lib/src/wallet/wallet.ts
Lines 1625 to 1627 in eeb9655
| */ | ||
| private assertWallet(): asserts this is { wallet: HathorWallet } { | ||
| if (!this.wallet) { | ||
| throw new TypeError('Wallet is required to build nano contract transactions.'); |
There was a problem hiding this comment.
It was the closest I could get to the type of the error. Since this can be only HathorWallet or null , I decided for a type error.
Do you think a simple Error would be more adequate here?
…-policy * origin/master: (31 commits) feat: fee token creation (#858) fix: checkAddressMine was crashing when called with an empty array (#977) feat: the nano amount field should accept 0 Merge pull request #975 from HathorNetwork/chore/bump-v2.11.0 feat: add nano execution logs API (#973) Refactor: Fullnode facade to Typescript (#972) Merge pull request #970 from HathorNetwork/chore/bump-v2.10.0 fix: added missing tx proposal delete schema (#969) feat: add missing api calls for graphviz and nano (#967) Merge pull request #965 from HathorNetwork/feat/complete-token-info-object chore: bump package to v2.9.1 (#962) feat: catch and store async promise error (#960) chore: bump wallet lib to v2.9.0 (#958) tests: use core v0.67.0 docker image for the integration tests (#956) tests: update integration tests blueprints for the new core sdk (#954) fix: added a minimal accessData so methods that need storage don't crash (#955) feat: fee header (#951) feat: added read-only start (#950) fix: dont add metadata changed for voided txs (#948) test: Wallet Service infrastructure (#909) ...
Summary
This PR starts the migration of the Fullnode Facade
HathorWalletinsrc/new/wallet.jsfrom Javascript to Typescript. This file has over 3400 lines and dozens of methods, so the best approach to do this refactor is with steps as small as possible.The first step is given by this PR:
.jsto.tsand fix all referencesany( permission to useanyadded in a file-wide scale )Rationale
Trying to fix any of the issues that arose or start typing would result in a PR difficult to review. This one, even though it changes many lines, is trivial to read as all of them set variables to
any.Fixing
anyassignments later is trivial because our linter forces us to do so. The work of assigning types to variables can be done with multiple independent easily reviewable PRs in parallel, as long as none of them try to change the same linesImmediate benefits
As of now, some of the differences between the
HathorWalletandHathorWalletServiceWalletare still unmapped. This PR maps them, adds comments for easy reference and provides the starting point to fix them in future PRsAcceptance Criteria
anySecurity Checklist