Merged 'Prepare Transaction' & 'Send Transaction' into single "Pay" Step #17
Conversation
WalkthroughThis PR consolidates the TransactionReview component's multi-step transaction flow into a single unified Pay flow, removing intermediate state management ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stablepay-sdk/src/widget/TransactionReview.jsx (2)
91-99: Guard against missingwalletClientand clear stale errors inhandlePay
handlePaybails out ifaccount/details/transactionare missing, but not ifwalletClientis null. In that casewalletClient.sendTransactionwill throw. Also,erroris never cleared on retry, so a previous failure keeps the “Show Details” button visible even after a successful send.I’d tighten the guard and reset the error before starting a new pay attempt:
- const handlePay = async () => { - if (!account || !contextTransactionDetails || !transaction) { + const handlePay = async () => { + if (!account || !walletClient || !contextTransactionDetails || !transaction) { setMessage("❌ Wallet not connected or transaction details missing"); return; } try { + setError(null); setMessage("⏳ Preparing transaction..."); @@ - setTxHash(txHash); - setMessage("✅ Transaction sent!"); + setTxHash(txHash); + setMessage("✅ Transaction sent!"); } catch (error) { setError(error); setMessage("❌ Transaction failed."); } };Also applies to: 145-156
91-96: Add in-flight state to prevent double-submitting the Pay transaction
handlePayhas no in-flight guard, and the Pay button stays enabled. On slow networks or if the user double-clicks, you can easily end up firing multiple non‑idempotentsendTransactioncalls for a single “Pay” action.Given this is a payment flow, it’s safer to introduce an
isPayingflag and disable the Pay button (and maybe the Connect button) while a transaction is being prepared/sent:- const [message, setMessage] = useState(""); + const [message, setMessage] = useState(""); + const [isPaying, setIsPaying] = useState(false); @@ const handlePay = async () => { @@ try { + setIsPaying(true); setMessage("⏳ Preparing transaction..."); @@ - setTxHash(txHash); - setMessage("✅ Transaction sent!"); + setTxHash(txHash); + setMessage("✅ Transaction sent!"); } catch (error) { setError(error); setMessage("❌ Transaction failed."); + } finally { + setIsPaying(false); } }; @@ - {account && ( - <button className={styles.walletButton} onClick={handlePay}> - Pay - </button> - )} + {account && ( + <button + className={styles.walletButton} + onClick={handlePay} + disabled={isPaying} + > + {isPaying ? "Processing..." : "Pay"} + </button> + )}Also applies to: 181-185
🧹 Nitpick comments (2)
stablepay-sdk/src/widget/TransactionReview.jsx (2)
49-53: VerifytradeDataBuyScshape; avoid passing full object toparseEther/UIFrom
setTransactionDetails({ tradeAmount: tradeData ? tradeData.amount : null, ... })it looks likehandleTradeDataBuyScreturns an object with anamountfield. InhandlePayand the “You Pay” display you treattradeDataBuyScas a primitive:
- Native path:
const amountToSend = tradeDataBuySc || "0";thenparseEther(String(amountToSend)).- UI:
{tradeDataBuySc ? tradeDataBuySc : "Calculating..."}.If
tradeDataBuyScis indeed the whole trade object, this will stringify to"[object Object]"in both the UI andparseEther, which is likely wrong.Consider storing just the numeric amount in state and using that consistently:
- let tradeData = null; - if (selectedToken.key === "native") { + let tradeData = null; + if (selectedToken.key === "native") { try { tradeData = await newTransaction.handleTradeDataBuySc(String(tokenAmount)); - setTradeDataBuySc(tradeData); + setTradeDataBuySc(tradeData.amount); @@ - const UI = "0x0232556C83791b8291E9b23BfEa7d67405Bd9839"; - const amountToSend = tradeDataBuySc || "0"; + const UI = "0x0232556C83791b8291E9b23BfEa7d67405Bd9839"; + const amountToSend = tradeDataBuySc ?? "0"; @@ - {selectedToken.key === "stablecoin" - ? `${contextTransactionDetails.amount} ${contextTransactionDetails.tokenSymbol}` - : `${tradeDataBuySc ? tradeDataBuySc : "Calculating..."} ${ + {selectedToken.key === "stablecoin" + ? `${contextTransactionDetails.amount} ${contextTransactionDetails.tokenSymbol}` + : `${tradeDataBuySc ?? "Calculating..."} ${ contextTransactionDetails.tokenSymbol }`}Also consider blocking the Pay action until
tradeDataBuyScis resolved to avoid sending a zero amount.Also applies to: 103-106, 168-173
142-147: Minor: ensure consistent indentation around “Sending transaction…” messageLine 143 (
setMessage("⏳ Sending transaction...");) is over-indented relative to surrounding lines, which makes the try block slightly harder to scan. Aligning it with the other statements in the block would improve readability.This is cosmetic and optional, but easy to tidy up while you’re here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stablepay-sdk/src/widget/TransactionReview.jsx(4 hunks)
🔇 Additional comments (2)
stablepay-sdk/src/widget/TransactionReview.jsx (2)
114-121: Recommendation to change"0"to0nis correct for viem v2 consistencyBased on viem v2 documentation,
encodeFunctionDatarequiresBigIntvalues foruint256parameters. TheparseUnitshelper returnsbigint, so the fallback should also bebigint(0n) instead of string ("0") to maintain type consistency and ensure proper encoding of ERC-20transferoperations.The suggested fix is sound:
- const amountToSend = contextTransactionDetails.amount - ? parseUnits( - String(contextTransactionDetails.amount), - contextTransactionDetails.stableCoinDecimals - ) - : "0"; + const amountToSend = contextTransactionDetails.amount + ? parseUnits( + String(contextTransactionDetails.amount), + contextTransactionDetails.stableCoinDecimals + ) + : 0n;
208-226: I've exhausted the available verification methods. The repository appears to be private or not publicly accessible, and the specific file cannot be located in public sources. Let me provide the rewritten review comment based on this finding:
Verify network configuration before hard-coding explorer URL
The transaction explorer link is currently hard-coded to ETC Mordor's Blockscout:
<a href={`https://blockscout.com/etc/mordor/tx/${txHash}`} ... > {txHash.slice(0, 6)}...{txHash.slice(-6)} </a>If this widget supports multiple networks (as suggested by
selectedNetworkandgetBlockchainDetails()), this will direct users to the wrong explorer on non-Mordor chains.Verify that:
getBlockchainDetails()or similar network configuration actually exposes per-network explorer base URLscontextTransactionDetailsor another context object is available with explorer configuration- Multi-network support is actually intended for this component
If confirmed, derive the explorer URL from network configuration rather than hard-coding it.
|
This PR fixes issue #16 |
| connectWallet, | ||
| account, | ||
| walletClient, | ||
| publicClient, |
There was a problem hiding this comment.
@shrestha-das why is the public client removed here?
There was a problem hiding this comment.
Hello @Tanya-ruby, actually the public client is removed because it's not used in this code.
There was a problem hiding this comment.
okay , makes sense since we do not use txdata anywhere.Although I see the explorer links functions with diffferent networks link was removed ,can you add that back ?
we would need to modify in such a way that the link is not just for mordor testnet but also other networks.
-> Another thing which will help is if you can add a screenshot or screenrecording of the changes.
Package the sdk locally and use the merchantdashboard repo and replace the stablepay sdk with the locally packged one and you can test it directly and also sharre the screenshots! @shrestha-das
There was a problem hiding this comment.
@Tanya-ruby , Can you please help me to figure out what other tests to do and screenshots to take here?
There was a problem hiding this comment.
okay , makes sense since we do not use txdata anywhere.Although I see the explorer links functions with diffferent networks link was removed ,can you add that back ?
we would need to modify in such a way that the link is not just for mordor testnet but also other networks.
So I will add these lines again
+ const getExplorerUrl = () => {
+ if (!txHash || !selectedNetwork) return null;
+
+ const explorerBaseUrls = {
+ "ethereum-classic": "https://etc-mordor.blockscout.com/tx/",
+ "sepolia": "https://sepolia.etherscan.io/tx/",
+ "milkomeda-mainnet": "https://explorer-mainnet-cardano-evm.c1.milkomeda.com/tx/",
+ };
+
+ return explorerBaseUrls[selectedNetwork]
+ ? `${explorerBaseUrls[selectedNetwork]}${txHash}`
+ : null;
+ };And modify here -
- <div className={styles.transactionLink}>
- ✅ Transaction Hash:{" "}
- <a
- href={`https://blockscout.com/etc/mordor/tx/${txHash}`}to -
+ {getExplorerUrl() && txHash && (
+ <div className={styles.transactionLink}>
+ ✅ Transaction Hash:{" "}
+ <a
+ href={`${getExplorerUrl}${txHash}`}- Is this okay?
There was a problem hiding this comment.
@shrestha-das that is the merchant dashboard , you can add and test the sdk in the merchant demo website ,that's where you can check the sdk working
2 participants
This PR updates the
TransactionReviewcomponent to simplify the transaction workflow. Previously, preparing and sending transactions were handled in separate steps. Now, a single Pay button prepares and sends the transaction in one click, improving user experience and reducing complexity.Changes Made
txDatastate as transactions are now sent immediately after preparation.handlePayfunction.Benefits
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.