Skip to content

Conversation

@br41nlet
Copy link
Contributor

@br41nlet br41nlet commented Oct 4, 2024

Note

Medium Risk
Touches core configuration/constants (network IDs, contracts, gas defaults) that can affect runtime behavior across environments, and introduces CI automation that may unexpectedly merge or fail builds if misconfigured.

Overview
Updates repository tooling and docs: adds .env.example, expands .gitignore, tweaks README.md formatting and points the demo link to v8/develop.

Refactors configuration/constants for v8 by removing the root constants.js (CommonJS) and introducing constants/constants.js (ESM) plus constants/index.js/constants/index.cjs exports; constants are updated/expanded (new chain IDs, updated RPC/contract addresses, new paranet/gas/fee-related settings, and larger MAX_FILE_SIZE).

Adds GitHub Actions workflows to automatically merge v8/develop into Test_Publish_Query and to fail CI when package-lock.json is missing/out-of-sync via npm ci --dry-run.

Written by Cursor Bugbot for commit b4af128. This will update automatically on new commits. Configure here.

@br41nlet br41nlet self-assigned this Oct 4, 2024
brkagithub and others added 28 commits December 26, 2024 16:08
8.0.0+001 release - Gnosis gas price fix
…tibility

V8 - allowance backwards compatibility
BLOCKCHAIN_IDS.NEUROWEB_MAINNET,
BLOCKCHAIN_IDS.HARDHAT_1,
BLOCKCHAIN_IDS.HARDHAT_2,
];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Orphaned array expression creates dead code

An array literal is created with blockchain IDs but never assigned to a variable or exported. This appears to be incomplete code that was meant to be NEUROWEB_INCENTIVE_TYPE_CHAINS, matching the equivalent export in constants/constants.js. The array expression evaluates but has no effect, suggesting this was accidentally committed or incompletely implemented.

Fix in Cursor Fix in Web


- name: Merge v8/develop into Test_Publish_Query
run: |
git merge origin/v8/develop --no-edit || echo "Merge failed but continuing"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Workflow silently masks merge failures before pushing

The merge command uses || echo "Merge failed but continuing" which causes the step to succeed even when the merge fails. The subsequent push step then executes, but since HEAD hasn't changed (due to the failed merge), nothing meaningful is pushed. The workflow reports success despite failing to actually merge the branches, which could mislead teams into thinking branches are synchronized when they aren't.

Fix in Cursor Fix in Web

@@ -0,0 +1 @@
PRIVATE_KEY=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Example file includes real private key

.env.example contains a concrete PRIVATE_KEY value. Even if intended for local Hardhat usage, it’s easy for users/CI to copy it into .env and accidentally reuse a known key outside local testing, leading to compromised funds or account control.

Fix in Cursor Fix in Web

@@ -0,0 +1,3 @@
import { BLOCKCHAIN_IDS } from './constants.js';

export { BLOCKCHAIN_IDS };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Public constants export is incomplete

package.json exposes ./constants, but constants/index.js/constants/index.cjs only export BLOCKCHAIN_IDS even though constants/constants.js defines many other configuration constants. This makes dkg.js/constants unusable for expected config values and can break consumers relying on exported constants beyond BLOCKCHAIN_IDS.

Additional Locations (1)

Fix in Cursor Fix in Web

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Comment on lines +15 to +38
if: github.event.pull_request.merged == true || github.event_name == 'push'
runs-on: ubuntu-latest

steps:
- name: Checkout target branch (Test_Publish_Query)
uses: actions/checkout@v3
with:
ref: Test_Publish_Query
token: ${{ secrets.GITHUB_TOKEN }}

- name: Set up Git identity
run: |
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"

- name: Fetch latest v8/develop
run: git fetch origin v8/develop

- name: Merge v8/develop into Test_Publish_Query
run: |
git merge origin/v8/develop --no-edit || echo "Merge failed but continuing"

- name: Push updated branch
run: git push origin HEAD:Test_Publish_Query No newline at end of file

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}

Copilot Autofix

AI about 2 months ago

To fix the issue, an explicit permissions block should be added to either the root of the workflow or to the individual job in the workflow YAML file. Since this workflow pushes commits to a branch (git push) and performs merges, it requires contents: write permission for repository contents (code, branches). The safest and most maintainable fix is to add a permissions block at the root of the YAML file (below name: and before on:), giving contents: write. This immediately clarifies what the workflow can do, prevents privilege escalation should defaults change, and adheres to GitHub's security guidelines.

Implementation Steps:

  • Edit .github/workflows/auto-merge-main-to-Test_Publish_Query.yml.
  • Insert the following after the name: line (line 1), before the on: block:
    permissions:
      contents: write

Suggested changeset 1
.github/workflows/auto-merge-main-to-Test_Publish_Query.yml

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/auto-merge-main-to-Test_Publish_Query.yml b/.github/workflows/auto-merge-main-to-Test_Publish_Query.yml
--- a/.github/workflows/auto-merge-main-to-Test_Publish_Query.yml
+++ b/.github/workflows/auto-merge-main-to-Test_Publish_Query.yml
@@ -1,4 +1,6 @@
 name: Auto Merge v8/develop into Test_Publish_Query
+permissions:
+  contents: write
 
 on:
   push:
EOF
@@ -1,4 +1,6 @@
name: Auto Merge v8/develop into Test_Publish_Query
permissions:
contents: write

on:
push:
Copilot is powered by AI and may make mistakes. Always verify output.
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

This PR is being reviewed by Cursor Bugbot

Details

Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

};

export const DEFAULT_PROXIMITY_SCORE_FUNCTIONS_PAIR_IDS = {
development: { 'hardhat1:31337': 2, 'hardhat2:31337': 2, 'otp:2043': 2 },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent blockchain config between related constants

Medium Severity

DEFAULT_PROXIMITY_SCORE_FUNCTIONS_PAIR_IDS.development includes 'otp:2043': 2, but BLOCKCHAINS.development no longer contains 'otp:2043' (it was removed in this commit). This creates an inconsistency where code looking up proximity score functions for development may expect otp:2043 to be a valid development blockchain, but it won't be found in BLOCKCHAINS.development.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.