refactor djed-sdk\src\djed\tradeUtils.js

[aniket866]

• Fixed transaction sending to consistently use Web3.js instead of Ethers.js syntax.

• Improved transaction verification by polling for the receipt with a timeout.

• Corrected UI fee unscaling to prevent double division and wrong fee values.

• No comment changes and minimal code modifications.

• Improves reliability and correctness of trade execution.

@Zahnentferner Please review

Summary by CodeRabbit

• Bug Fixes

  • Improved transaction verification using a polling-based timeout to more reliably confirm receipts.
  • Refined fee scaling for more accurate UI fee display.

• Chores

  • Updated transaction submission handling with improved error handling and validation.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@aniket866 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 3 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

Added a new exported constant FEE_UI_UNSCALED; changed promiseTx parameter name (introducing a mismatch between signature and body); and replaced single delayed receipt fetch with a polling-based verifyTx that waits up to 60,000 ms and resolves with receipt.status or rejects on timeout/error.

Changes

Cohort / File(s)	Summary
Transaction utilities djed-sdk/src/djed/tradeUtils.js	Added FEE_UI_UNSCALED via decimalUnscaling(FEE_UI.toString(), SCALING_DECIMALS); changed promiseTx parameter name from signer to Singer (function body still references signer — likely runtime error); replaced verifyTx with a polling loop that checks getTransactionReceipt until a 60,000 ms timeout and resolves with receipt.status or rejects on timeout/error.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Caller as Caller
participant promiseTx as promiseTx
participant verifyTx as verifyTx (poller)
participant Blockchain as Blockchain / Node

Caller->>promiseTx: call(tx, isWalletConnected, Singer)
promiseTx->>verifyTx: submit tx / start verify
verifyTx->>Blockchain: getTransactionReceipt(txHash) [poll]
Blockchain-->>verifyTx: receipt | null
alt receipt found
    verifyTx-->>promiseTx: receipt.status
    promiseTx-->>Caller: resolve(status)
else timeout / error
    verifyTx-->>promiseTx: reject(error/timeout)
    promiseTx-->>Caller: reject(error)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Refactor djed-sdk\src\djed\tradeUtils.js #77: Matches objectives — fixes around UI fee unscaling, promiseTx behavior, and polling-based verifyTx.

Suggested reviewers

• Zahnentferner

Poem

🐰 I hopped through code with careful paws,
Fixed fee scales and watched the claws,
Polling beats a single wait,
Transactions now check their fate,
Hooray — a tidy hop and applause! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is vague and generic, using only 'refactor' without specifying what was actually changed or improved in the tradeUtils.js file.	Consider a more specific title that reflects the main changes, such as 'Fix transaction verification polling and UI fee calculation in tradeUtils' to clearly communicate the primary improvements.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@djed-sdk/src/djed/tradeUtils.js`:
- Around line 123-131: The exported function promiseTx changed its third
parameter from signer to web3 (signature now promiseTx(isWalletConnected, tx,
web3)), but the compiled bundles still export the old signature; rebuild the
compiled output so dist/ bundles reflect the new API, then run the build step
(e.g., npm/yarn build) to regenerate the ESM/UMD artifacts and verify the
generated bundles export promiseTx with the web3 parameter and the same runtime
behavior (ensure web3?.eth is checked and web3.eth.sendTransaction is used).

🧹 Nitpick comments (2)

djed-sdk/src/djed/tradeUtils.js (2)

137-153: Clarify receipt.status semantics for callers.

The function resolves with receipt.status, which is false for reverted transactions and true for successful ones. This means a resolved promise doesn't guarantee transaction success—callers must check the returned boolean.

Consider either:

1. Rejecting when receipt.status === false to make failure handling explicit
2. Documenting this behavior for callers

♻️ Option: Reject on failed transactions

       const receipt = await web3.eth.getTransactionReceipt(hash);
       if (receipt) {
-        return resolve(receipt.status);
+        if (receipt.status) {
+          return resolve(receipt.status);
+        }
+        return reject(new Error("Transaction reverted"));
       }

---

134-135: Consider extracting timeout as a named constant.

The 60-second timeout is reasonable for blockchain confirmation. For consistency with CONFIRMATION_WAIT_PERIOD, consider defining this as a named constant (e.g., CONFIRMATION_TIMEOUT) in the constants file.

[coderabbitai]

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 (1)

djed-sdk/src/djed/tradeUtils.js (1)

123-130: ⚠️ Potential issue | 🔴 Critical

Fix promiseTx parameter mismatch (ReferenceError).
Line 123 declares Singer but the body references signer, which throws before any Promise is returned.

🐛 Proposed fix

-export const promiseTx = (isWalletConnected, tx, Singer) => {
+export const promiseTx = (isWalletConnected, tx, signer) => {
   if (!isWalletConnected) {
     return Promise.reject(new Error("Metamask not connected!"));
   }
   if (!signer) {
     return Promise.reject(new Error("Couldn't get Signer"));
   }
   return signer.sendTransaction(tx);
 };