-
Notifications
You must be signed in to change notification settings - Fork 9
Added formatting to checksum addresses #195
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
Added formatting to checksum addresses #195
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/invoice-dashboard/src/utils/formatAddress.ts (2)
1-2: Consider consolidating address validation logicThe code currently uses both
checkAddressfrom shared utils andgetAddressfrom viem for validation. SincegetAddressalready performs address validation and checksumming, we could simplify the code by removing the redundant check.-import { checkAddress } from "@requestnetwork/shared-utils/checkEthAddress"; - if (!checkAddress(address)) { - console.error("Invalid address!"); - } + try { + const checksumAddress = getAddress(address); + return `${checksumAddress.slice(0, first)}...${checksumAddress.slice(-last)}`; + } catch (error) { + console.error("Invalid address!", error); + return address; + }
Line range hint
8-11: Improve error handling for invalid addressesThe current implementation logs an error but continues execution, which could lead to runtime errors if
getAddressreceives an invalid address. Consider implementing proper error handling with a fallback behavior.- if (!checkAddress(address)) { - console.error("Invalid address!"); - } - - const checksumAddress = getAddress(address); + try { + const checksumAddress = getAddress(address); + return `${checksumAddress.slice(0, first)}...${checksumAddress.slice(-last)}`; + } catch (error) { + console.error("Invalid address:", error); + return address; // Return original address as fallback + }packages/create-invoice-form/src/lib/utils/prepareRequest.ts (1)
Ethereum address checksumming needs to be applied consistently
The codebase has inconsistent handling of Ethereum addresses. While the reviewed file correctly uses
getAddressfor checksumming, there are other instances that need attention:
packages/payment-widget/src/lib/utils/request.ts: Uses raw addresses without checksumming forsellerAddress,payerAddress, andfeeAddressshared/utils/initCurrencyManager.ts: UsestoLowerCase()for address comparison instead of proper checksummingshared/utils/formatAddress.ts: Some instances use raw addresses without checksummingshared/utils/getCurrency.ts: Uses raw address from currency.value without checksumming🔗 Analysis chain
Line range hint
1-127: Verify consistent address handling across the codebaseLet's ensure all Ethereum addresses in the codebase are handled consistently with checksumming.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other potential Ethereum address fields that might need checksumming # Look for potential address fields or variables rg -i "address|ethereum|0x" --type ts # Look for direct address assignments without getAddress ast-grep --pattern 'value: $address' # Look for other instances where addresses might be used rg -i "type.*ethereum.*address" --type tsLength of output: 12321
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/create-invoice-form/src/lib/utils/prepareRequest.ts(4 hunks)packages/invoice-dashboard/src/utils/formatAddress.ts(2 hunks)
🔇 Additional comments (2)
packages/create-invoice-form/src/lib/utils/prepareRequest.ts (2)
1-1: LGTM! Appropriate use of viem's getAddress
The addition of getAddress from viem is the correct choice for implementing EIP-55 checksum validation.
37-37: LGTM! Consistent address checksumming
The changes correctly apply checksumming to all Ethereum addresses in the request parameters, ensuring consistency throughout the system.
Also applies to: 41-41, 52-52, 125-125
Fixes: PR
Problem
The Invoice Dashboard occasionally displays non-checksummed Ethereum addresses, which can lead to inconsistencies and reduced clarity. This issue arises because addresses retrieved from IPFS may be in lowercase.
Changes Made
Summary by CodeRabbit
New Features
Bug Fixes
Documentation