Merged
Conversation
ghost
self-assigned this
ghost
self-requested a review
as a 
Contributor
📝 WalkthroughWalkthroughRemoves tag-based data feed processing from the v0.1 address-generation script, simplifying outputs to a single others-addresses.json. In the PoR app, removes early checks for zero decimals and denominator, proceeding with computations based on adapter.Decimals without explicit zero-guard conditions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as CLI/User
participant S as generate-contract-addresses.js
participant FS as File System
U->>S: Run script with deployments folder
S->>FS: Read deployments per network
note over S,FS: Aggregator_* and AggregatorProxy_* skipped
S->>S: Normalize contract names (trim trailing _ segments)
S->>FS: Write other-addresses.json
note right of S: dataFeeds output removed
sequenceDiagram
autonumber
participant R as Reporter
participant A as PoR App (New, shouldReport)
participant M as Math
R->>A: New(ctx)
A->>A: Build entry without Decimals==0 check
R->>A: shouldReport(e, lastInfo, newVal, fetchedTime)
A->>M: denominator = 10^Decimals (no zero guard)
M-->>A: scaled old/new values
A-->>R: decision to report
note right of A: Previous zero-denominator check removed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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
🧹 Nitpick comments (3)
contracts/v0.1/scripts/generate-contract-addresses.js (3)
31-33: Use path-relative + path.sep for cross‑platform network extraction.Apply:
- const network = filePath.replace(deploymentsPath, '').split('/')[0] + const relative = path.relative(deploymentsPath, filePath) + const network = relative.split(path.sep)[0]
51-51: Validate addresses (format and non-zero) before writing.Add helper:
// place near top-level helpers const isValidAddress = (addr) => typeof addr === 'string' && /^0x[a-fA-F0-9]{40}$/.test(addr) && addr.toLowerCase() !== '0x0000000000000000000000000000000000000000'And guard assignment:
- others[network][contractName] = address + if (!isValidAddress(address)) { + console.warn(`Skipping invalid address in ${filePath}: ${address}`) + return + } + others[network][contractName] = address
43-51: No collisions found; still recommend defensive normalization + overwrite warningVerified: ran the duplicate-check against contracts/v0.1/deployments (baobab, cypress) — no duplicate base names found. Update the normalization to remove all whitespace and warn on overwrites.
File: contracts/v0.1/scripts/generate-contract-addresses.js Lines: 43-51
- const splitted = contractName.replace(' ', '').split('_') - if (splitted.length > 1) { - splitted.pop() - contractName = splitted.join('_') - } - others[network][contractName] = address + const nameNoSpace = contractName.replace(/\s+/g, '') + const parts = nameNoSpace.split('_') + const baseName = parts.length > 1 ? parts.slice(0, -1).join('_') : nameNoSpace + if (others[network][baseName] && others[network][baseName] !== address) { + console.warn(`Overwriting ${network}/${baseName}: ${others[network][baseName]} -> ${address}`) + } + others[network][baseName] = address
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (45)
contracts/v0.1/deployments/baobab/PORProxy_AAPL_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/PORProxy_AMZN_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/PORProxy_GOOGL_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/PORProxy_META_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/PORProxy_MSFT_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/PORProxy_NVDA_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/PORProxy_TSLA_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_AAPL_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_AMZN_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_GOOGL_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_META_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_MSFT_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_NVDA_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/baobab/POR_TSLA_20250912150912.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_AAPL_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_AMZN_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_GOOGL_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_META_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_MSFT_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_NVDA_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/PORProxy_TSLA_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_AAPL_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_AMZN_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_GOOGL_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_META_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_MSFT_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_NVDA_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/cypress/POR_TSLA_20250912150954.jsonis excluded by!**/*.jsoncontracts/v0.1/deployments/other-addresses.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151144_AAPL.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151159_MSFT.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151206_AMZN.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151213_GOOGL.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151235_META.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151246_NVDA.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/20250912151253_TSLA.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/baobab/POR/migration.lockis excluded by!**/*.lock,!**/*.lockcontracts/v0.1/migration/cypress/POR/20250912151313_AAPL.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/20250912151323_MSFT.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/20250912151332_AMZN.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/20250912151339_GOOGL.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/20250912151350_META.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/20250912151402_NVDA.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/20250912151410_TSLA.jsonis excluded by!**/*.jsoncontracts/v0.1/migration/cypress/POR/migration.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
contracts/v0.1/scripts/generate-contract-addresses.js(5 hunks)node/pkg/por/app.go(0 hunks)
💤 Files with no reviewable changes (1)
- node/pkg/por/app.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: martinkersner
PR: Bisonai/orakl#0
File: :0-0
Timestamp: 2024-07-27T01:26:16.468Z
Learning: The `fetchTags` function in `contracts/v0.1/scripts/generate-contract-addresses.js` was refactored for better error handling and data processing, as per the changes committed in https://github.com/Bisonai/orakl/pull/1364/commits/13f8132c6c41acf08e63c60b0f279f903d944bdc.
Learnt from: martinkersner
PR: Bisonai/orakl#1244
File: contracts/v0.2/src/Feed.sol:0-0
Timestamp: 2024-07-27T01:26:16.468Z
Learning: The `changeOracles` function in the `Feed.sol` contract includes input validation to skip adding zero addresses, as per the user's fix.
📚 Learning: 2024-07-27T01:26:16.468Z
Learnt from: martinkersner
PR: Bisonai/orakl#0
File: :0-0
Timestamp: 2024-07-27T01:26:16.468Z
Learning: The `fetchTags` function in `contracts/v0.1/scripts/generate-contract-addresses.js` was refactored for better error handling and data processing, as per the changes committed in https://github.com/Bisonai/orakl/pull/1364/commits/13f8132c6c41acf08e63c60b0f279f903d944bdc.
Applied to files:
contracts/v0.1/scripts/generate-contract-addresses.js
🧬 Code graph analysis (1)
contracts/v0.1/scripts/generate-contract-addresses.js (1)
contracts/v0.2/script/collect-addresses.cjs (1)
network(49-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: core-build
- GitHub Check: build
🔇 Additional comments (5)
contracts/v0.1/scripts/generate-contract-addresses.js (5)
63-64: Return shape LGTM.
77-78: Main flow LGTM (tags removed).
3-3: Remove unused importaxios(verify repo‑wide before removal).
contracts/v0.1/scripts/generate-contract-addresses.js — remove:-const axios = require('axios')Sandbox search failed (ripgrep skipped files / script error). Confirm no other usage locally:
git grep -n axios || rg -n --hidden -S '\baxios\b' --glob '!node_modules/**'
14-16: Signature simplified — safe: no external v0.1 callers depend on removedtagsarg.Only two-arg usage found is in contracts/v0.2/script/collect-addresses.cjs (it defines/uses readDeployments(folderPath, tags)); contracts/v0.1/scripts/generate-contract-addresses.js and contracts/v0.1/utils/index.ts use the single-arg signature. No action required.
79-82: Keep JSON.stringify when calling storeJson; unify the filename to avoid cross-version breakage.
- storeJson writes the provided data verbatim (see contracts/v0.1/scripts/utils.cjs — it calls writeFileSync with the passed data); callers in this repo already pass JSON.stringify, so removing the stringify would be incorrect and break existing callers (e.g., contracts/v0.1/scripts/generate-contract-addresses.js, contracts/v0.1/scripts/admin-aggregator/generate-aggregator-deployments.cjs).
- Filename mismatch: v0.1 script writes "other-addresses.json" (contracts/v0.1/scripts/generate-contract-addresses.js) while v0.2 expects "others-addresses.json" (contracts/v0.2/script/collect-addresses.cjs and contracts/v0.2/README.md) — pick one convention and update the scripts/README accordingly.
Likely an incorrect or invalid review comment.
Comment on lines
+28
to
+30
| if (file.startsWith('Aggregator_') || file.startsWith('AggregatorProxy_')) { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Skipping Aggregator — confirm no consumer still expects datafeeds outputs.*
This removes datafeeds entirely. Ensure nothing reads datafeeds-addresses.json or Aggregator-derived mappings.
🏁 Script executed:
#!/bin/bash
rg -n 'datafeeds-addresses\.json|AggregatorProxy_|Aggregator_' -g '!**/node_modules/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "--- Top-level files ---"
ls -la | sed -n '1,200p' || true
echo "--- .git present? ---"
if [ -d .git ]; then echo "FOUND .git"; else echo "NO .git"; fi
echo "--- Git branch (if any) ---"
git rev-parse --abbrev-ref HEAD 2>/dev/null || true
echo "--- Tool availability ---"
command -v rg >/dev/null 2>&1 && rg --version || echo "rg: not available"
command -v git >/dev/null 2>&1 && git --version || echo "git: not available"
command -v ast-grep >/dev/null 2>&1 && ast-grep --version || echo "ast-grep: not available"
command -v fd >/dev/null 2>&1 && fd --version || echo "fd: not available"
TARGET="contracts/v0.1/scripts/generate-contract-addresses.js"
echo "--- Check target file ---"
if [ -f "$TARGET" ]; then
echo "FOUND: $TARGET"
echo "------ $TARGET (first 300 lines with line numbers) ------"
nl -ba "$TARGET" | sed -n '1,300p'
else
echo "NOT FOUND: $TARGET"
fi
echo "--- Search repo with rg (no ignore) ---"
if command -v rg >/dev/null 2>&1; then
rg -n --no-ignore -S 'datafeeds-addresses\.json|datafeeds-addresses|AggregatorProxy_|Aggregator_' || true
else
echo "Skipping rg search (rg not installed)"
fi
echo "--- Search repo with git grep ---"
if command -v git >/dev/null 2>&1; then
git grep -n -- 'datafeeds-addresses' || true
git grep -n -- 'AggregatorProxy_' || true
git grep -n -- 'Aggregator_' || true
else
echo "Skipping git grep (git not available)"
fi
echo "--- Fallback grep ---"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git -e 'datafeeds-addresses' -e 'AggregatorProxy_' -e 'Aggregator_' . || true
echo "--- Search for literal filename datafeeds-addresses.json ---"
if command -v rg >/dev/null 2>&1; then
rg -n --no-ignore -S 'datafeeds-addresses\.json' || true
else
grep -RIn --exclude-dir=node_modules --exclude-dir=.git -e 'datafeeds-addresses.json' . || true
fiLength of output: 1917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Show target file (contracts/v0.1/scripts/generate-contract-addresses.js) ----"
sed -n '1,400p' contracts/v0.1/scripts/generate-contract-addresses.js || true
echo
echo "---- ripgrep (search for datafeeds / Aggregator references) ----"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore -S -e 'datafeeds-addresses\.json' -e 'datafeeds-addresses' -e 'datafeeds' -e 'AggregatorProxy_' -e 'Aggregator_' || true
else
echo "rg: not available"
fi
echo
echo "---- git grep fallback ----"
if command -v git >/dev/null 2>&1; then
git grep -n -- 'datafeeds-addresses' || true
git grep -n -- 'AggregatorProxy_' || true
git grep -n -- 'Aggregator_' || true
else
echo "git: not available"
fi
echo
echo "---- find literal file ----"
find . -type f -iname 'datafeeds-addresses.json' -print || true
echo
echo "---- grep fallback ----"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git -e 'datafeeds-addresses' -e 'datafeeds' -e 'AggregatorProxy_' -e 'Aggregator_' . || trueLength of output: 8926
Restore or separately emit Aggregator/datafeeds addresses — consumers still expect them.
if (file.startsWith('Aggregator_') || file.startsWith('AggregatorProxy_')) {
return
}
- Evidence: core/src/worker/data-feed.utils.ts, core/src/worker/data-feed-L2.ts, core/src/listener/data-feed-L2.ts (import Aggregator__factory); contracts/v0.2/script/collect-addresses.cjs (references "datafeeds-addresses.json"); contracts/v0.2/script/prepare-submission-proxy-migration.py; contracts/v0.2/README.md; contracts/v0.1/deploy/Aggregator/Aggregator.cjs (creates Aggregator_* / AggregatorProxy_* names).
- Action: either stop skipping Aggregator_* here or add a dedicated step that emits datafeeds-addresses.json (and update/remove consumers accordingly).
🤖 Prompt for AI Agents
contracts/v0.1/scripts/generate-contract-addresses.js lines 28-30: the current
early return skips files named Aggregator_* and AggregatorProxy_ which prevents
emitting datafeed addresses consumers expect; either remove this conditional so
Aggregator_* and AggregatorProxy_* entries are included in the main output, or
keep the skip but add a new step that collects those Aggregator entries into a
separate datafeeds-addresses.json (same format as other outputs) and write that
file to the same output directory; update any downstream references if you
choose the new file approach.
This pull request was closed.
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.
Description
also cleaned up unused addr generation
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment