Conversation
…UpdateTests This commit improves the logic for retrieving broadcaster update requests from pending transactions within the BroadcasterUpdateTests class. The updated implementation ensures that the correct operation type is checked against pending transactions, allowing for more robust identification of broadcaster update requests. This enhancement addresses potential issues with unrelated pending transactions and improves the clarity of the test's output when a broadcaster update request is created.
feat: enhance broadcaster update transaction retrieval in Broadcaster…
Contributor
📝 WalkthroughWalkthroughA test utility for broadcaster updates now robustly searches across all pending transactions to locate BROADCASTER_UPDATE operations using case-insensitive matching, replacing simplistic transaction handling in two test flows. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
Contributor
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 `@scripts/sanity/secure-ownable/broadcaster-update-tests.cjs`:
- Around line 482-495: The current loop over pending transactions returns the
first BROADCASTER_UPDATE matching expectedOpType, which may pick a stale
request; modify the search in the block that calls
this.callContractMethod(this.contract.methods.getTransaction(txId)) so it
filters transactions by the same requester identity used by
createBroadcasterRequestAndDeriveTxId() and iterates newest-first (reverse the
pending array) before returning; specifically ensure you check
txRecord.params.requester (or the equivalent requester field) matches the
requester used when creating the request and only then return the txRecord when
op matches expectedOpType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4790b4e9-779f-4185-88ff-2f0060850e44
📒 Files selected for processing (1)
scripts/sanity/secure-ownable/broadcaster-update-tests.cjs
Comment on lines
+482
to
+495
| if (pending && pending.length > 0) { | ||
| for (const txId of pending) { | ||
| const txRecord = await this.callContractMethod(this.contract.methods.getTransaction(txId)); | ||
| const op = txRecord && txRecord.params && txRecord.params.operationType; | ||
| if ( | ||
| op && | ||
| (op === expectedOpType || | ||
| (typeof op === 'string' && | ||
| typeof expectedOpType === 'string' && | ||
| op.toLowerCase() === expectedOpType.toLowerCase())) | ||
| ) { | ||
| console.log(` ✅ Broadcaster update request created (txId: ${txRecord.txId || txId})`); | ||
| return txRecord; | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t return the first matching broadcaster-update tx.
This helper creates a new request immediately above, but Lines 483-494 return the first pending BROADCASTER_UPDATE they find. If another broadcaster-update request is already pending, the test can attach to the stale tx instead of the one it just created. createBroadcasterRequestAndDeriveTxId() already scopes its fallback by requester and searches newest-first; this helper should apply the same constraint here.
💡 Proposed fix
for (let i = 0; i < 5; i++) {
const pending = await this.callContractMethod(this.contract.methods.getPendingTransactions());
if (pending && pending.length > 0) {
- for (const txId of pending) {
+ let newestMatch = null;
+ for (const txId of pending) {
const txRecord = await this.callContractMethod(this.contract.methods.getTransaction(txId));
const op = txRecord && txRecord.params && txRecord.params.operationType;
+ const requester = txRecord && txRecord.params && txRecord.params.requester;
if (
op &&
(op === expectedOpType ||
(typeof op === 'string' &&
typeof expectedOpType === 'string' &&
- op.toLowerCase() === expectedOpType.toLowerCase()))
+ op.toLowerCase() === expectedOpType.toLowerCase())) &&
+ requester &&
+ requester.toLowerCase() === this.roles.owner.toLowerCase()
) {
- console.log(` ✅ Broadcaster update request created (txId: ${txRecord.txId || txId})`);
- return txRecord;
+ if (!newestMatch || BigInt(txRecord.txId) > BigInt(newestMatch.txId)) {
+ newestMatch = txRecord;
+ }
}
}
+ if (newestMatch) {
+ console.log(` ✅ Broadcaster update request created (txId: ${newestMatch.txId})`);
+ return newestMatch;
+ }
}📝 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.
Suggested change
| if (pending && pending.length > 0) { | |
| for (const txId of pending) { | |
| const txRecord = await this.callContractMethod(this.contract.methods.getTransaction(txId)); | |
| const op = txRecord && txRecord.params && txRecord.params.operationType; | |
| if ( | |
| op && | |
| (op === expectedOpType || | |
| (typeof op === 'string' && | |
| typeof expectedOpType === 'string' && | |
| op.toLowerCase() === expectedOpType.toLowerCase())) | |
| ) { | |
| console.log(` ✅ Broadcaster update request created (txId: ${txRecord.txId || txId})`); | |
| return txRecord; | |
| } | |
| if (pending && pending.length > 0) { | |
| let newestMatch = null; | |
| for (const txId of pending) { | |
| const txRecord = await this.callContractMethod(this.contract.methods.getTransaction(txId)); | |
| const op = txRecord && txRecord.params && txRecord.params.operationType; | |
| const requester = txRecord && txRecord.params && txRecord.params.requester; | |
| if ( | |
| op && | |
| (op === expectedOpType || | |
| (typeof op === 'string' && | |
| typeof expectedOpType === 'string' && | |
| op.toLowerCase() === expectedOpType.toLowerCase())) && | |
| requester && | |
| requester.toLowerCase() === this.roles.owner.toLowerCase() | |
| ) { | |
| if (!newestMatch || BigInt(txRecord.txId) > BigInt(newestMatch.txId)) { | |
| newestMatch = txRecord; | |
| } | |
| } | |
| } | |
| if (newestMatch) { | |
| console.log(` ✅ Broadcaster update request created (txId: ${newestMatch.txId})`); | |
| return newestMatch; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity/secure-ownable/broadcaster-update-tests.cjs` around lines 482
- 495, The current loop over pending transactions returns the first
BROADCASTER_UPDATE matching expectedOpType, which may pick a stale request;
modify the search in the block that calls
this.callContractMethod(this.contract.methods.getTransaction(txId)) so it
filters transactions by the same requester identity used by
createBroadcasterRequestAndDeriveTxId() and iterates newest-first (reverse the
pending array) before returning; specifically ensure you check
txRecord.params.requester (or the equivalent requester field) matches the
requester used when creating the request and only then return the txRecord when
op matches expectedOpType.
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.
Summary by CodeRabbit