feat: remove any types from packages/caravan-clients#194
feat: remove any types from packages/caravan-clients#194AvhiMaz wants to merge 1 commit intocaravan-bitcoin:mainfrom
Conversation
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@bucko13 please. have a look, thank you! |
will do! thanks. things are a bit backed up at the moment but definitely will try and get to this asap. |
|
Hey @AvhiMaz ! Thanks a ton for the PR—really appreciate the effort! Just dropping in some initial review comments for a few tweaks. Let me know what you think! " |
Legend101Zz
left a comment
Legend101Zz
left a comment
There was a problem hiding this comment.
Some initial reviews for the bitcoind.ts and blockchain.ts files
| } catch (e) { | ||
| console.log("send tx error", e); | ||
| throw (e.response && e.response.data.error.message) || e; | ||
| if (e instanceof Error) { |
There was a problem hiding this comment.
We can use any for the catch block, as the trade-off between strict typing and readability is reasonable in this case. Ensuring a precise error type may not provide significant benefits given the context, while any keeps the implementation clean and maintainable. ... you can find more discussion on this here
| true, | ||
| ]); | ||
| return response.result; | ||
| return response.result as TransactionResponse; |
There was a problem hiding this comment.
could you please check the Bitcoin Core docs to verify whether TransactionResponse includes all the expected fields? If any fields are missing, feel free to extend the type accordingly.
Sorry for the extra trouble, but this will help us cover the scope of the issue, particularly:
Creating a reference table mapping Bitcoin RPC methods to TypeScript interfaces.
Really appreciate the help :)
| ); | ||
| } catch (e: any) { | ||
| throw (e.response && e.response.data) || e; | ||
| } catch (e) { |
There was a problem hiding this comment.
same for this as stated in above comment
| }; | ||
| } catch (e: any) { | ||
| throw (e.response && e.response.data) || e; | ||
| } catch (e) { |
| ): Promise<{ txid: string; vout: number; amountSats: BigNumber }[]> { | ||
| if (client.type === BLOCK_EXPLORER) { | ||
| return blockExplorerGetAddresesUTXOs(address, network); | ||
| return blockExplorerGetAddresesUTXOs(address, network).then((utxos) => |
There was a problem hiding this comment.
Looking into the code, I noticed that both blockExplorerGetAddressesUTXOs and bitcoindListUnspent return FormattedUTXO. In the case of bitcoindListUnspent, the run is hardcoded, so we could potentially adjust that.
Is there a specific reason for modifying the function’s return type by introducing loops to traverse UTXOs and return { txid: string; vout: number; amountSats: BigNumber }[]? Given that neither blockExplorerGetAddressesUTXOs nor bitcoindListUnspent return vout directly, but instead expose it as index: utxo.vout, this change seems unnecessary.
I'd recommend keeping the function’s core logic intact while refining the types, as altering its structure might introduce unintended issues across the package where such changes aren't required.
|
|
||
| if (!isBitcoindClient(client)) { | ||
| throw new Error("Invalid bitcoind client configuration"); | ||
| if (isBitcoindClient(client)) { |
There was a problem hiding this comment.
same here ... we should not need to change the function for correcting the types
| // calculate the total balance from all utxos | ||
| const balanceSats = utxos | ||
| .map((utxo) => utxo.amountSats) | ||
| .map((utxo: { amountSats: BigNumber }) => utxo.amountSats) |
There was a problem hiding this comment.
It looks like the suggested change was likely intended to address this issue. However, these modifications are unnecessary. Instead, we can simply define unsortedUTXOs as FormattedUTXO[], which aligns with the expected structure.
Additionally, changing unsortedUTXOs: { amountSats: BigNumber }[] | undefined; breaks the following usage:
const utxos = sortInputs(unsortedUTXOs);This happens because sortInputs expects each UTXO to have a txid, as seen in its implementation:
export function sortInputs(inputs) {
return inputs.sort((input1, input2) => {
if (input1.txid > input2.txid) {
return 1;
} else if (input1.txid < input2.txid) {
return -1;
} else {
return input1.index < input2.index ? -1 : 1;
}
});
}Since sortInputs relies on txid and index, the additional traversal logic seems unnecessary. Keeping unsortedUTXOs typed as FormattedUTXO[] should resolve the issue without modifying the function's structure.
🧩 Analysis chain🏁 Script executed: Length of output: 1011 🏁 Script executed: Length of output: 3113 🏁 Script executed: Length of output: 30853 @Legend101Zz I'll review the PR that removes "any" types from packages/caravan-clients. 🏁 Script executed: Length of output: 5581 🏁 Script executed: Length of output: 865 🏁 Script executed: Length of output: 4628 @Legend101Zz I've completed my re-review of the PR that removes "any" types from packages/caravan-clients. The PR looks good overall and successfully accomplishes its goal of replacing Main improvements:
Remaining items:
The changes will improve code reliability, maintainability, and help catch type errors at compile time rather than runtime. The PR is ready to move forward. ✅ Actions performedReview triggered.
|
WalkthroughThe changes update several modules to enhance type safety and standardize error handling. Functions across the codebase now use stricter type definitions, replacing generic types with specific ones for returned objects and parameters. Error handling has been refined by checking for Axios-specific errors and ensuring that errors are appropriately identified as instances of Error before processing. Updates include more explicit return types for functions that send transactions, fetch UTXOs, and interact with blockchain data, along with additional properties in transaction and UTXO interfaces. Some logical adjustments ensure that data structures such as UTXOs and transaction details adhere to a consistent format across different client implementations. These changes are applied consistently across multiple files, ensuring that public APIs and exported entities reflect the updated type safety and error handling mechanisms. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/caravan-clients/src/blockExplorer.tsOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/caravan-clients".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/caravan-clients/.eslintrc.js » @caravan/eslint-config/library.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/caravan-clients/src/bitcoind.tsOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/caravan-clients".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/caravan-clients/.eslintrc.js » @caravan/eslint-config/library.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/caravan-clients/src/blockchain.tsOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/caravan-clients".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/caravan-clients/.eslintrc.js » @caravan/eslint-config/library.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/caravan-clients/src/blockchain.ts (1)
120-126: Improved type annotations in reduce function.The updated type annotations in the reduce function make the expected types more explicit, although the annotation for the accumulator could be simplified. Consider using
BigNumberdirectly as the accumulator type since that's what it actually is.Consider simplifying the accumulator type annotation:
- ( - accumulator: { plus: (arg0: BigNumber) => BigNumber }, - currentValue: BigNumber, - ) => accumulator.plus(currentValue), + (accumulator: BigNumber, currentValue: BigNumber) => + accumulator.plus(currentValue),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/caravan-clients/src/bitcoind.ts(2 hunks)packages/caravan-clients/src/blockExplorer.ts(5 hunks)packages/caravan-clients/src/blockchain.ts(3 hunks)packages/caravan-clients/src/client.ts(14 hunks)packages/caravan-clients/src/types.ts(2 hunks)packages/caravan-clients/src/wallet.ts(8 hunks)
🔇 Additional comments (28)
packages/caravan-clients/src/types.ts (3)
232-237: Great enhancement to the TransactionResponse interface.Adding these detailed transaction properties (
size,weight,vin,vout, andresult) provides better type safety for transaction data and will help catch potential type errors at compile time rather than runtime. This aligns perfectly with the PR's goal of removinganytypes.
169-169: Excellent replacement of 'any' with 'unknown'.Using
unknownrather thananyfor the params property is a good practice as it forces type checking before usage, preventing unsafe operations without warnings. The comment explains this reasoning clearly.
287-287: Good addition of the vout property to FormattedUTXO.This properly extends the interface to include the transaction output index, which is necessary for correct UTXO handling. This addition ensures type consistency across the codebase when dealing with UTXOs.
packages/caravan-clients/src/blockExplorer.ts (5)
53-53: Good addition of vout property to return object.Adding the
voutproperty to the returned object fromblockExplorerGetAddresesUTXOsensures consistency with the updatedFormattedUTXOinterface in types.ts. This makes the function's return type more accurate and complete.
64-69: Improved error handling with proper type checking.Replacing the generic error handling with proper Axios error type checking improves type safety. This approach correctly identifies and handles Axios-specific errors while maintaining the ability to throw other types of errors.
98-103: Consistent error handling pattern.The error handling is consistently implemented across the file, following the same pattern as in the previous function. This consistency makes the code more maintainable.
124-129: Consistent error handling pattern.The error handling is consistently implemented across the file, following the same pattern as in the previous functions. This consistency makes the code more maintainable.
149-154: Consistent error handling pattern.The error handling is consistently implemented across the file, following the same pattern as in the previous functions. This consistency makes the code more maintainable.
packages/caravan-clients/src/bitcoind.ts (2)
243-252: Improved error handling with proper instance checks.The updated error handling in
bitcoindSendRawTransactionis more robust, first checking ifeis an instance ofErrorand then checking if it's an Axios error with a response. This provides better type safety than usinganyand improves error reporting.
322-328: Enhanced type safety with specific return type.Updating the return type from
Promise<any>toPromise<TransactionResponse>and properly casting the result improves type safety and ensures the data structure is consistent with the interface definition. This change helps prevent runtime errors by catching type mismatches during compilation.packages/caravan-clients/src/blockchain.ts (4)
44-44: Enhanced return type with specific structure.Replacing
Promise<any[]>with a more specific return type providing exact properties (txid,vout,amountSats) improves type safety and code clarity. This change helps ensure consistent UTXO structure throughout the application.
46-52: Well-structured UTXO mapping for block explorer client.The explicit mapping ensures that all necessary properties are included and properly typed, with a nullish coalescing operator to provide a default value for
voutwhen undefined. This approach provides better type safety and consistency.
55-66: Improved bitcoind client handling with proper type checking.Using the
isBitcoindClienttype guard before accessing bitcoind-specific properties and mapping the UTXOs to a consistent structure ensures type safety. The consistent property mapping between block explorer and bitcoind clients simplifies downstream processing.
84-84: Specific type for unsortedUTXOs variable.Replacing
any[]with{ amountSats: BigNumber }[]provides better type information for the variable, ensuring that it has at least theamountSatsproperty needed for balance calculation.packages/caravan-clients/src/wallet.ts (6)
7-7: Strong type safety improvement for BitcoindWalletClientError constructor.Nice improvement by explicitly typing the constructor parameter as
string | undefinedinstead of using an implicitanytype.
21-21: Good replacement ofanywithunknowntype.Using
unknown[] | Record<string, unknown>instead ofany[] | Record<string, any>is safer as it requires type checking before operating on the values, preventing potential runtime errors.
114-128: Improved type safety for address status check.Good improvement by adding a type check before comparing the result value. This makes the code more robust against unexpected response formats.
166-169: Enhanced return type with additional properties.Adding the
vout,block_height, andblock_hashproperties to the return type improves the API by providing more complete transaction data.
190-190: Improved Promise array typing.Good replacement of
Promise<any>[]withPromise<unknown>[]for better type safety.
212-225: Good use of type assertions for structured data.The addition of explicit type assertions when extracting data from
previousTransactionsimproves type safety and clarifies the expected structure of the data.packages/caravan-clients/src/client.ts (8)
61-82: Excellent improvement in removinganytypes from normalizeTransactionData.The explicit object types for
vinandvoutarrays greatly enhance type safety and code clarity. This change makes the intention of the code more evident and helps prevent type-related errors.
110-127: Improved error handling with Axios error detection.Good enhancement to the Request method by properly detecting Axios errors and throwing appropriate responses. The change from
data?: anytodata?: objectalso improves type safety.
210-235: Better type safety in getAddressUtxos with explicit return type.Changing the return type from
Promise<any>toPromise<UTXO[]>and improving error handling with Error instance checking makes the code more robust and self-documenting.
333-348: Enhanced broadcastTransaction with proper return type and error handling.The function now correctly returns
Promise<string>instead ofPromise<any>and has improved error handling that differentiates between Error instances and other unknown errors.
398-401: Consistent error handling pattern.Good improvement to check if the error is an instance of Error before accessing its properties. This helps prevent runtime errors when dealing with unexpected error types.
478-483: Standardized error handling pattern across methods.The consistent approach to error handling (checking if error is an instance of Error) across multiple methods improves code reliability and maintainability.
524-542: Improved getTransactionHex with stronger typing and error handling.The change from
Promise<any>toPromise<string>return type and better error handling with proper type checking ensures more reliable API usage.
566-571: Consistent error handling in getTransaction.The error handling pattern matches the improvements made throughout the file, enhancing the overall robustness of the codebase.
| // @ts-expect-error Will Fix this | ||
| return callBitcoind(url.toString(), auth, method, params); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
TS-expect-error needs fixing
The comment indicates this is a temporary workaround that needs fixing. Consider prioritizing the actual fix to remove the need for this directive.
🏁 Script executed:
#!/bin/bash
# Check for other @ts-expect-error comments in the codebase
rg "@ts-expect-error" --type tsLength of output: 560
Action Required: Remove Temporary TS-Expect-Error Directive in Production Code
The production file at packages/caravan-clients/src/wallet.ts still contains a temporary @ts-expect-error directive (lines 35-36). This is a workaround that masks a type issue with the callBitcoind invocation. Unlike the acceptable usage in tests, production code should not rely on such temporary fixes. Please investigate the underlying type mismatch and implement a proper solution so that the @ts-expect-error directive can be removed safely.
- File:
packages/caravan-clients/src/wallet.ts(lines 35-36)
Legend101Zz
left a comment
Legend101Zz
left a comment
There was a problem hiding this comment.
Some more review comments ...
I'd like you to focus on two things :
-
Try to limit changes strictly to type definitions to avoid any unintended side effects as Altering existing functionality, even slightly, could introduce breaking changes across the entire package.
-
The catch block
anycomments I mentioned
| return response.data; | ||
| } catch (e: any) { | ||
| throw (e.response && e.response.data) || e; | ||
| } catch (e) { |
There was a problem hiding this comment.
Sorry for pointing it out repeatedly... but again I guess we can leave any's for the catch block
| } | ||
|
|
||
| private async Request(method: Method, path: string, data?: any) { | ||
| private async Request(method: Method, path: string, data?: object) { |
There was a problem hiding this comment.
Ok ... so this would break existing functionality...
public async Post(path: string, data?: object) {
return this.Request("POST", path, data);
}Changing the parameter type from any to object would prevent passing strings to the Post method, which would break this functionality.
public async broadcastTransaction(rawTx: string): Promise<any> {
try {
if (this.type === ClientType.PRIVATE) {
return bitcoindSendRawTransaction({
hex: rawTx,
...this.bitcoindParams,
});
}
return await this.Post(`/tx`, rawTx); // rawTx is a string, not an object
} catch (error: any) {
throw new Error(`Failed to broadcast transaction: ${error.message}`);
}
}One idea can be to use a union type instead ( But honestly I am not a fan of this , I guess we can do better):
public async Post(path: string, data?: string | object | FormData) {
return this.Request("POST", path, data);
}| } | ||
|
|
||
| public async Post(path: string, data?: any) { | ||
| public async Post(path: string, data?: object) { |
There was a problem hiding this comment.
I left a comment above telling how can it potentially break some functionalities
| address, | ||
| ...this.bitcoindParams, | ||
| }); | ||
| return unspent.map((utxo) => ({ |
There was a problem hiding this comment.
looks good ... can you please check that bitcoindListUnspent returns index instead of vout so
vout: utxo.index and looking at
export interface UTXO {
txid: string;
vout: number;
value: number;
status: {
confirmed: boolean;
block_time: number;
};
}where are we getting the extra block_height and block_hash fields and also I see bitcoindListUnspent not returning these fields :
Promise<
{
txid: string;
amount: string;
amountSats: string;
index: number;
confirmed: boolean;
transactionHex: string;
time: string;
}[]can you please check
| }, | ||
| })); | ||
| } | ||
| return await this.Get(`/address/${address}/utxo`); |
There was a problem hiding this comment.
If possible can also cross reference the mempool and blockstream API docs and check if what type you are expecting they are returning that only
| transaction.vout.push({ | ||
| scriptPubkeyHex: output.scriptPubKey.hex, | ||
| scriptPubkeyAddress: output.scriptPubKey.address, | ||
| scriptPubkeyHex: output.scriptpubkey, |
There was a problem hiding this comment.
Please try to limit changes strictly to type definitions to avoid any unintended side effects. Altering existing functionality, even slightly, could introduce breaking changes across the entire package.
The goal should be to enhance type safety while keeping modifications to the core logic as minimal as possible. If a functional change is absolutely necessary, it would be great to discuss it first to assess the potential impact.
| try { | ||
| if (this.type === ClientType.PRIVATE) { | ||
| return await callBitcoind<TransactionResponse>( | ||
| const response = await callBitcoind<TransactionResponse>( |
| * Interface for formatted UTXO data | ||
| */ | ||
| export interface FormattedUTXO { | ||
| vout: number; |
There was a problem hiding this comment.
please try to avoid changes like these as they would break the package in many places as discussed above
| } | ||
|
|
||
| export interface TransactionResponse { | ||
| size: number; |
There was a problem hiding this comment.
Why was it needed to add more fields here ??
| amountSats: bitcoinsToSatoshis(amount.toString()), | ||
| transactionHex: previousTransactions[mapindex].result.hex, | ||
| time: previousTransactions[mapindex].result.blocktime, | ||
| transactionHex: ( |
There was a problem hiding this comment.
again this looks like a breaking change to me ... can you please verify if this won't break anything
|
@AvhiMaz if you have time to address the comments, i might have some availability to review and hopefully merge this week. |
|
Hi @AvhiMaz, just checking in to see if you’ve had a chance to go through the pending review comments on the PR. Let me know if you need any help or clarification :) We’ve been reviewing and cleaning up some long-standing PRs to reduce the backlog, and I noticed this one is still pending. Just wanted to give it a bump—do you think you’ll have time to address the remaining comments soon? |
|
Closing for inactivity. |
3 participants
No description provided.