Fix: Replace Ethers.js sendTransaction with Web3.js implementation in…#91
Open
Rav1Chauhan wants to merge 2 commits intoDjedAlliance:mainfrom
Open
Fix: Replace Ethers.js sendTransaction with Web3.js implementation in…#91Rav1Chauhan wants to merge 2 commits intoDjedAlliance:mainfrom
Rav1Chauhan wants to merge 2 commits intoDjedAlliance:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@djed-sdk/src/djed/tradeUtils.js`:
- Around line 121-136: The promiseTx function should validate that tx.from is
present (reject with an Error if missing) and set the gas field using standard
Web3 naming and sensible precedence (use tx.gas if provided, else tx.gasLimit,
else 500000) when calling web3.eth.sendTransaction; update the sendTransaction
payload to include gas: selectedGas and keep other fields (from, to, value,
data) intact; also keep in mind web3.eth.sendTransaction returns a PromiEvent
(not an Ethers TransactionResponse) so callers relying on .hash/.wait() must be
adjusted accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
djed-sdk/src/djed/tradeUtils.js (1)
136-147:⚠️ Potential issue | 🟠 MajorAvoid wrapping
sendTransactionin a plainPromise.This strips the returned PromiEvent interface (
transactionHash/confirmation listeners) and can regress existing transaction-lifecycle handling.Suggested change
- return new Promise((resolve, reject) => { - web3.eth - .sendTransaction({ - from: tx.from, - to: tx.to, - value: tx.value, - data: tx.data, - gas: selectedGas, - }) - .on("receipt", resolve) - .on("error", reject); - }); + return web3.eth.sendTransaction({ + from: tx.from, + to: tx.to, + value: tx.value, + data: tx.data, + gas: selectedGas, + });#!/bin/bash # Verify whether promiseTx consumers rely on tx-hash timing or PromiEvent methods. rg -n -C3 --type js '\bpromiseTx\s*\(' djed-sdk/src rg -n -C3 --type js '\.wait\s*\(|\.hash\b|transactionHash|\.on\(\s*["'\'']transactionHash["'\'']' djed-sdk/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@djed-sdk/src/djed/tradeUtils.js` around lines 136 - 147, The current promiseTx implementation wraps web3.eth.sendTransaction in a plain Promise which discards the PromiEvent API (transactionHash/confirmation listeners); update promiseTx so it returns the original PromiEvent from web3.eth.sendTransaction (or returns that PromiEvent after attaching .on('receipt', resolve) and .on('error', reject) handlers) instead of a new Promise, preserving transactionHash and confirmation listeners; locate the promiseTx function in djed-sdk/src/djed/tradeUtils.js and change the return to pass through the sendTransaction PromiEvent rather than wrapping it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@djed-sdk/src/djed/tradeUtils.js`:
- Around line 121-147: The bundled distribution still contains the old
promiseTx(isWalletConnected, tx, signer) implementation; regenerate the compiled
bundle so djed-sdk/dist/esm/index.js reflects the updated promiseTx using
web3.eth.sendTransaction with event handlers (.on("receipt",
resolve).on("error", reject)). Rebuild the package (run the project’s build
script or transpiler) to produce a new dist, verify that the exported promiseTx
in djed-sdk/dist/esm/index.js matches the source implementation (checks for
web3.eth and selectedGas logic and the .on handlers), and commit the regenerated
dist alongside the source change.
---
Duplicate comments:
In `@djed-sdk/src/djed/tradeUtils.js`:
- Around line 136-147: The current promiseTx implementation wraps
web3.eth.sendTransaction in a plain Promise which discards the PromiEvent API
(transactionHash/confirmation listeners); update promiseTx so it returns the
original PromiEvent from web3.eth.sendTransaction (or returns that PromiEvent
after attaching .on('receipt', resolve) and .on('error', reject) handlers)
instead of a new Promise, preserving transactionHash and confirmation listeners;
locate the promiseTx function in djed-sdk/src/djed/tradeUtils.js and change the
return to pass through the sendTransaction PromiEvent rather than wrapping it.
…ect gas precedence
Rav1Chauhan
force-pushed
the
fix/web3-promiseTx-consistency
branch
from
f8c9cdd to
623a06d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… promiseTx
Addressed Issues:
Fixes #68
Screenshots/Recordings:
Not applicable.
This change updates internal transaction handling logic in
djed-sdk/src/djed/tradeUtils.js and does not affect UI components.
The fix was verified by testing transaction submission using a standard Web3 provider.
Additional Notes:
🔍 Problem
The promiseTx helper used:
signer.sendTransaction(tx)
This is Ethers.js syntax, but the rest of the SDK consistently uses Web3.js contract objects and providers.
When using a Web3 provider, this resulted in:
sendTransaction is undefined
because Web3 providers do not expose sendTransaction in the same way as Ethers signers.
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit