-
Notifications
You must be signed in to change notification settings - Fork 0
chore: prepare build version #34
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
base: development
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates introduce a new environment variable and configuration for a Tari cold wallet address, propagate this value through the app, and enhance the management of pending bridge transactions. Type definitions, state management, and modal props are extended to track pending transactions, while logging and error handling are standardized and improved. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainModal
participant Store
participant TariL1Signer
participant Config
User->>MainModal: Open modal
MainModal->>Store: Retrieve pendingBridgeTxFromTU
Store->>TariL1Signer: isPendingTappletTx()
TariL1Signer-->>Store: SendOneSidedRequest | undefined
Store-->>MainModal: pendingBridgeTxFromTU
MainModal->>Config: Get TARI_BRIDGE_COLDWALLET_ADDRESS
MainModal->>User: Display modal with transaction info
Poem
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (6)
components/modals/connection-modal/connection-modal.tsx (1)
14-14
: Remove duplicate cursor styleThe className contains both
hover:cursor-pointer
andcursor-pointer
which is redundant.- className="text-black font-bold hover:cursor-pointer absolute top-4 right-4 cursor-pointer flex text-xl rounded-full p-1 bg-black/10 hover:bg-black/20" + className="text-black font-bold absolute top-4 right-4 cursor-pointer flex text-xl rounded-full p-1 bg-black/10 hover:bg-black/20".env.example (1)
3-3
: Consider adding documentation for the new environment variableThe addition of the new environment variable is good, but consider adding a comment to indicate the expected format (e.g., a Tari wallet address format) and any validation requirements.
NEXT_PUBLIC_WALLET_CONNECT_PROJECT_ID='' NEXT_PUBLIC_BACKEND_API_URL=http://localhost:3001 +# Tari bridge cold wallet address in the standard Tari wallet format NEXT_PUBLIC_TARI_BRIDGE_COLDWALLET_ADDRESS=''
hooks/use-bridge-to-ethereum-fees/use-bridge-to-ethereum-fees.ts (1)
24-24
: Improved error message capitalization, but missing standardized prefixThe capitalization change from "Eth" to "ETH" is a good consistency improvement. However, based on the surrounding changes in other files, consider adding the "[ TAPPLET-BRIDGE ]" prefix to fully align with the logging standardization effort.
- console.error('Could not estimate ETH fees: ', error) + console.error('[ TAPPLET-BRIDGE ] Could not estimate ETH fees: ', error)components/modals/main-modal/main-modal.tsx (2)
30-34
: Consider extracting the conditional amount logic.The same conditional logic for determining which amount to use is duplicated in both the
feesData
calculation and in theWrapModal
amount prop. This could be extracted to a local variable to improve maintainability.const { isConnected } = useAccount() const modalRef = useRef<HTMLDivElement>(null) + const effectiveAmount = !isBridging && pendingBridgeTxFromTU + ? pendingBridgeTxFromTU?.amount + : amount - const feesData = useBridgeToEthereumFees( - !isBridging && pendingBridgeTxFromTU - ? pendingBridgeTxFromTU?.amount - : amount, - ) + const feesData = useBridgeToEthereumFees(effectiveAmount)And then update line 83-87 as well:
<WrapModal closeModal={closeModal} - amount={ - !isBridging && pendingBridgeTxFromTU - ? pendingBridgeTxFromTU?.amount - : amount - } + amount={effectiveAmount}
41-45
: Remove commented-out code.Consider removing this commented-out code if it's no longer needed, or uncomment it if the functionality should be restored. Keeping commented-out code in the codebase can lead to confusion about the intended behavior.
clients/tari-l1-signer.ts (1)
100-120
: Enhance method documentation.The new methods have basic JSDoc comments, but they could be improved with more specific information about:
- What constitutes a "pending" transaction
- What exactly
isPendingTappletTx
returns when there is a pending transaction- What happens if
removePendingTappletTx
is called with an invalid or non-existent paymentId/** * @description check if there is any processing transaction + * @returns The pending transaction details if one exists, undefined otherwise */ public async isPendingTappletTx(): Promise<SendOneSidedRequest | undefined> { return this.sendRequest({ methodName: 'isPendingTappletTx', args: [], }) } /** - * @description check if there is any processing transaction + * @description Remove a pending Tapplet transaction by its paymentId + * @param paymentId The ID of the pending transaction to remove * @returns true if removal was successful, false otherwise */ public async removePendingTappletTx(paymentId: string): Promise<boolean> { return this.sendRequest({ methodName: 'removePendingTappletTx', args: [paymentId], }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
bridge-v0.1.0.zip
is excluded by!**/*.zip
📒 Files selected for processing (12)
.env.example
(1 hunks)app/page.tsx
(3 hunks)clients/tari-l1-signer.ts
(3 hunks)components/modals/connection-modal/connection-modal.tsx
(1 hunks)components/modals/main-modal/main-modal.tsx
(3 hunks)components/modals/main-modal/main-modal.types.ts
(2 hunks)config/index.ts
(1 hunks)hooks/use-bridge-to-ethereum-fees/use-bridge-to-ethereum-fees.ts
(1 hunks)hooks/use-bridge-to-ethereum.ts
(3 hunks)store/account.ts
(2 hunks)store/signer.ts
(1 hunks)utils/config.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
components/modals/main-modal/main-modal.types.ts (1)
clients/tari-l1-signer.ts (1)
SendOneSidedRequest
(11-15)
hooks/use-bridge-to-ethereum.ts (1)
config/index.ts (1)
config
(1-10)
components/modals/main-modal/main-modal.tsx (1)
hooks/use-bridge-to-ethereum-fees/use-bridge-to-ethereum-fees.ts (1)
useBridgeToEthereumFees
(7-33)
store/account.ts (2)
types/tapplet/account.ts (1)
AccountData
(1-4)clients/tari-l1-signer.ts (1)
SendOneSidedRequest
(11-15)
🔇 Additional comments (18)
utils/config.ts (1)
7-7
: Appropriate linting suppression for global variableAdding the ESLint disable comment here is correct, as
var
is needed for the global variable declaration. This complies with JavaScript's global variable behavior, wherevar
declarations are properly hoisted to the global scope.components/modals/connection-modal/connection-modal.tsx (1)
12-12
: Good addition of therelative
positioning classAdding the
relative
class is appropriate since it establishes a proper positioning context for the absolutely positioned close button. This follows CSS best practices for modal design.store/signer.ts (1)
13-16
: Improved logging with standardized format and better informationThis is a good enhancement that:
- Uses the appropriate log level (info vs warn)
- Adds the standardized "[ TAPPLET-BRIDGE ]" prefix
- Includes specific information about which signer is being set
These changes will make logs more informative and easier to filter when debugging.
components/modals/main-modal/main-modal.types.ts (1)
1-1
: Clean type extension to support pending bridge transactionsThe addition of the
pendingBridgeTxFromTU
optional prop correctly extends the interface to support the new functionality for tracking pending Tari-to-Ethereum bridge transactions.Also applies to: 17-17
app/page.tsx (3)
38-38
: Correctly added pending transaction state extractionThe addition of
pendingBridgeTxFromTU
to the destructured properties from theuseTariAccount
hook is correct and necessary to support the pending bridge transaction tracking functionality.
124-124
: Standardized logging formatGood improvement to use the consistent "[ TAPPLET-BRIDGE ]" prefix in log messages, making them more identifiable and filterable.
157-157
: Correctly passing pending transaction data to modalThe
pendingBridgeTxFromTU
prop is now correctly passed to theMainModal
component, allowing the modal to adjust its display and behavior based on pending transaction state.components/modals/main-modal/main-modal.tsx (1)
98-98
: Confirm intent to disable outside clicks.The removed click handler from this div means users can no longer close the modal by clicking outside it. If this is intentional (e.g., to prevent accidental closures during transactions), this is fine. Otherwise, consider restoring the outside click functionality.
hooks/use-bridge-to-ethereum.ts (3)
1-1
: Good improvement: Using dynamic configuration.Replacing hardcoded values with configuration values improves flexibility across different environments.
Also applies to: 14-14
38-38
: Consistent logging pattern applied.Using
console.debug
with a standardized prefix[ TAPPLET-BRIDGE ]
improves log filtering and readability.Also applies to: 44-44
47-50
: Better parameter naming.Explicit parameter naming improves code readability and maintainability.
store/account.ts (4)
11-11
: Good addition: Tracking pending Tapplet transactions.Adding the
pendingBridgeTxFromTU
state property enables the application to track and display pending bridge transactions from Tari Universe.Also applies to: 31-31
26-26
: Improved initialization: Using empty string instead of hardcoded address.Using an empty string for the initial address is cleaner than using a hardcoded value.
37-37
: Consistent logging pattern applied.Using standardized log prefixes and appropriate log levels (info/error) improves debugging capabilities.
Also applies to: 41-41, 47-47
53-56
: Ensure state consistency between transaction flags.The code now sets
isProcessingTransaction: !!pendingTx
based on pending Tapplet transactions, but there's also the existingpendingBridgeTx
array that could have entries. Make sure these two transaction status indicators don't conflict.Consider clarifying the relationship between
pendingBridgeTxFromTU
andpendingBridgeTx
- are they tracking different types of transactions? If so, theisProcessingTransaction
flag should consider both sources:set({ tariAccount: { account_id: account.account_id, address: account.address, }, available_balance: balance?.available_balance ?? 0, pendingBridgeTxFromTU: pendingTx, - isProcessingTransaction: !!pendingTx, + isProcessingTransaction: !!pendingTx || state.pendingBridgeTx.length > 0, })This would ensure that
isProcessingTransaction
accurately reflects both types of pending transactions.clients/tari-l1-signer.ts (3)
11-15
: Good change: Explicit interface with mandatory paymentId.Changing from an optional
message
to a mandatorypaymentId
field makes the transaction tracking requirements more explicit and enforced by the type system.
66-68
: Improved null safety with nullish coalescing.Adding nullish coalescing operators provides better defensive programming against unexpected API responses.
78-86
: Updated parameter name for consistency.The parameter name change from
message
topaymentId
aligns with the interface change.
TARI_BRIDGE_COLDWALLET_ADDRESS: | ||
process.env.NEXT_PUBLIC_TARI_BRIDGE_COLDWALLET_ADDRESS ?? '', |
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.
💡 Verification agent
🧩 Analysis chain
Validate the cold wallet address before use
The new configuration property is a good addition, moving from a hardcoded value to a configurable environment variable. However, consider adding validation to ensure the address is properly formatted when used in the application to prevent runtime errors if it's left empty or improperly configured.
🏁 Script executed:
#!/bin/bash
# Search for where this new config property is actually used
rg "TARI_BRIDGE_COLDWALLET_ADDRESS" --type ts
# Check to see if there's any validation before using the address
rg "TARI_BRIDGE_COLDWALLET_ADDRESS.*(validate|check|verify|empty|null|undefined)" --type ts
Length of output: 357
Add validation for TARI_BRIDGE_COLDWALLET_ADDRESS before use
To avoid passing an empty or malformed address into your bridge hook, validate the environment variable’s presence and format at import time (or at its point of use). Otherwise you risk runtime failures when the address is missing or invalid.
• In config/index.ts (lines 8–9), wrap the raw string in a check. For example:
// config/index.ts
const rawColdWallet = process.env.NEXT_PUBLIC_TARI_BRIDGE_COLDWALLET_ADDRESS;
if (!rawColdWallet) {
throw new Error(
'Environment variable NEXT_PUBLIC_TARI_BRIDGE_COLDWALLET_ADDRESS is required'
);
}
if (!/^0x[a-fA-F0-9]{40}$/.test(rawColdWallet)) {
throw new Error(
`Invalid TARI_BRIDGE_COLDWALLET_ADDRESS format: ${rawColdWallet}`
);
}
export const TARI_BRIDGE_COLDWALLET_ADDRESS = rawColdWallet;
• In hooks/use-bridge-to-ethereum.ts, you can safely assume the address is valid, or add a runtime guard if desired:
// hooks/use-bridge-to-ethereum.ts
if (!config.TARI_BRIDGE_COLDWALLET_ADDRESS) {
throw new Error('Cold wallet address is not set');
}
return useBridge({ address: config.TARI_BRIDGE_COLDWALLET_ADDRESS, … });
This ensures any missing or malformed address is caught early, preventing obscure failures deeper in the bridge logic.
🤖 Prompt for AI Agents
In config/index.ts around lines 8 to 9, the environment variable
TARI_BRIDGE_COLDWALLET_ADDRESS is used without validation, which risks runtime
errors if it is empty or malformed. Fix this by extracting the raw environment
variable into a constant, then check if it is present; if not, throw an error
indicating the variable is required. Next, validate the format against a regex
pattern matching the expected address format (e.g., /^0x[a-fA-F0-9]{40}$/); if
invalid, throw an error specifying the invalid format. Finally, export the
validated address constant for safe use elsewhere in the application.
if (!isSend) { | ||
console.error('[ TAPPLET-BRIDGE ] send one sided failed') | ||
} | ||
|
||
const { success } = await confirmTokenSent.mutateAsync(paymentId) | ||
console.log('[TAPPLET] confirm token sent success: ', success) | ||
if (!success) { | ||
console.error('[ TAPPLET-BRIDGE ] confirm token sent failed') | ||
} |
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.
🛠️ Refactor suggestion
Consider adding user-facing error notifications.
You've added good error handling with console logging, but there's no user-facing notification when these operations fail. Consider adding toast notifications or other UI feedback for these error cases.
if (!isSend) {
console.error('[ TAPPLET-BRIDGE ] send one sided failed')
+ // Show user-facing notification
+ // e.g., toast.error('Failed to send transaction. Please try again.')
}
const { success } = await confirmTokenSent.mutateAsync(paymentId)
if (!success) {
console.error('[ TAPPLET-BRIDGE ] confirm token sent failed')
+ // Show user-facing notification
+ // e.g., toast.error('Failed to confirm transaction. Please contact support.')
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if (!isSend) { | |
console.error('[ TAPPLET-BRIDGE ] send one sided failed') | |
} | |
const { success } = await confirmTokenSent.mutateAsync(paymentId) | |
console.log('[TAPPLET] confirm token sent success: ', success) | |
if (!success) { | |
console.error('[ TAPPLET-BRIDGE ] confirm token sent failed') | |
} | |
if (!isSend) { | |
console.error('[ TAPPLET-BRIDGE ] send one sided failed') | |
// Show user-facing notification | |
// e.g., toast.error('Failed to send transaction. Please try again.') | |
} | |
const { success } = await confirmTokenSent.mutateAsync(paymentId) | |
if (!success) { | |
console.error('[ TAPPLET-BRIDGE ] confirm token sent failed') | |
// Show user-facing notification | |
// e.g., toast.error('Failed to confirm transaction. Please contact support.') | |
} |
🤖 Prompt for AI Agents
In hooks/use-bridge-to-ethereum.ts around lines 52 to 59, the error handling
currently only logs errors to the console without notifying the user. To fix
this, integrate a user-facing notification system such as toast notifications in
both error cases: when isSend is false and when confirmTokenSent.mutateAsync
returns an unsuccessful response. This will provide immediate feedback to users
about the failure, improving the user experience.
Fixes plenty of small front bugs, especially store & modal for mending tx
Summary by CodeRabbit
New Features
Improvements
Style
Chores