Skip to content

feat: rm old code#802

Merged
cristianizzo merged 3 commits into
stagingfrom
development
Aug 3, 2025
Merged

feat: rm old code#802
cristianizzo merged 3 commits into
stagingfrom
development

Conversation

@cristianizzo

Copy link
Copy Markdown
Contributor

📝 Summary

Explain the purpose of these changes and what problem they solve.

Task: APP-0000

🔄 What Changed

💡 Use GitHub Copilot's "Generate Summary" button to auto-generate this section

✅ Checklist

🔍 Code Review

  • Self-reviewed all code changes
  • Ask AI/Copilot code review
  • Removed all debug code, console.logs, and dead code
  • Implemented error handling with try/catch blocks where needed

🧪 Testing

  • All test suites pass locally
  • Added comprehensive unit tests for new features
  • Included integration tests for end-to-end scenarios
  • Successfully tested on remote server

🔒 Security

  • Verified no secrets or credentials are exposed
  • Reviewed for common security vulnerabilities
  • Verified commit authorship - Reviewed all commits to ensure they're from team members (check for compromised accounts)

* feat: rm tokenHolderSync and dead code

* feat: lint

* feat: lint

* feat: lint

* feat: lint

* feat: lint
* feat: rm tokenHolderSync and dead code

* feat: lint

* feat: lint

* feat: lint

* feat: lint

* feat: lint

* feat: format
* feat: mig

* feat: mig

* feat: unit test

* feat: unit test

* feat: unit test

* feat: unit test

* feat: unit test

* feat: syncall use requeue

* feat: test
@cristianizzo cristianizzo marked this pull request as ready for review August 3, 2025 09:16
Copilot AI review requested due to automatic review settings August 3, 2025 09:16
@cristianizzo cristianizzo merged commit 11edbc8 into staging Aug 3, 2025
6 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes outdated token synchronization code by eliminating the complex token holder sync functionality and related concepts. The changes simplify the codebase by removing specialized sync tag handling, large token threshold checks, and transfer-based synchronization logic.

Key changes:

  • Removed the entire TokenHolderSync service and related token sync tag concepts
  • Simplified token synchronization to use standard blockchain log crawling only
  • Cleaned up configuration and test files to remove references to removed functionality

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/toolLauncher.ts Removed ManualSyncProposals import and added IntegrityToolMemberTransaction
tools/syncProposals.ts Complete file removal of the old manual sync proposals tool
tools/syncProposalTotalSupply.ts Updated to use token clockMode and blockTimestamp parameters
tools/integrityCheck/memberTransaction.ts New integrity checking tool for member transactions using Dune Analytics
src/services/aragon-plugins/tokenHolderSync.ts Complete removal of complex token holder sync logic
src/services/aragon-plugins/logTokenVoting.ts Simplified to use standard token sync without threshold checks
src/types/indexer.ts Removed ITokenSyncTagName enum and simplified type definitions
src/helpers/configIndexer.ts Removed sync tag related methods and simplified token service builders

NEED_CONNECTIONS: [EnumConnection.MONGODB],
DUNE_API_KEY: process.env.DUNE_API_KEY,
DUNE_API_BASE_URL: 'https://api.dune.com/api/v1',
NETWORK_FILTER: NetworksEnum.ethereumMainnet, // Default network, can be changed manually

Copilot AI Aug 3, 2025

Copy link

Choose a reason for hiding this comment

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

Hard-coded default network should be configurable through environment variables instead of requiring manual code changes.

Suggested change
NETWORK_FILTER: NetworksEnum.ethereumMainnet, // Default network, can be changed manually
NETWORK_FILTER: (() => {
const envNetwork = process.env.NETWORK_FILTER;
if (envNetwork && Object.values(NetworksEnum).includes(envNetwork as NetworksEnum)) {
return envNetwork as NetworksEnum;
} else {
if (envNetwork) {
logger.warn(
`Invalid NETWORK_FILTER "${envNetwork}" provided. Falling back to default: ${NetworksEnum.ethereumMainnet}`,
llo({})
);
}
return NetworksEnum.ethereumMainnet;
}
})(),

Copilot uses AI. Check for mistakes.
Comment on lines +234 to +237
// You need to create a query on Dune Analytics first and get the query ID
// For now, let's use a pre-existing query ID or create one
// This is a placeholder - you'll need to replace with actual query ID
const DUNE_QUERY_ID = process.env.DUNE_QUERY_ID || '3686182' // Replace with your actual query ID

Copilot AI Aug 3, 2025

Copy link

Choose a reason for hiding this comment

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

Hard-coded fallback query ID should be removed or documented. The comment suggests this is a placeholder that needs replacement.

Suggested change
// You need to create a query on Dune Analytics first and get the query ID
// For now, let's use a pre-existing query ID or create one
// This is a placeholder - you'll need to replace with actual query ID
const DUNE_QUERY_ID = process.env.DUNE_QUERY_ID || '3686182' // Replace with your actual query ID
// You must set the DUNE_QUERY_ID environment variable to your Dune Analytics query ID.
const DUNE_QUERY_ID = process.env.DUNE_QUERY_ID
if (!DUNE_QUERY_ID) {
throw new Error('DUNE_QUERY_ID environment variable is not set. Please set it to your Dune Analytics query ID.')
}

Copilot uses AI. Check for mistakes.
Comment on lines +302 to +306
'Dune query not configured. Create a query on Dune Analytics with this SQL and set DUNE_QUERY_ID:\n' +
'SELECT COUNT(*) as delegate_votes_changed_count\n' +
'FROM ethereum.logs\n' +
"WHERE topic0 = '0xdec2bacdd2f05b59de34da9b523dff8be42e5e38e818c82fdb0bae774387a724'\n" +
"AND LOWER(contract_address) = LOWER('{{token_address}}')",

Copilot AI Aug 3, 2025

Copy link

Choose a reason for hiding this comment

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

The error message contains SQL code which could be confusing in logs and doesn't provide actionable guidance for the specific environment setup needed.

Suggested change
'Dune query not configured. Create a query on Dune Analytics with this SQL and set DUNE_QUERY_ID:\n' +
'SELECT COUNT(*) as delegate_votes_changed_count\n' +
'FROM ethereum.logs\n' +
"WHERE topic0 = '0xdec2bacdd2f05b59de34da9b523dff8be42e5e38e818c82fdb0bae774387a724'\n" +
"AND LOWER(contract_address) = LOWER('{{token_address}}')",
'Dune query not configured. Please create a query on Dune Analytics to count delegate vote changes for the specified token address, and set the DUNE_QUERY_ID environment variable to your query\'s ID. See the documentation for guidance.'

Copilot uses AI. Check for mistakes.
Comment on lines 17 to +21
proposal.snapshot.totalSupply = await GovernanceErc20Helper.getPastTotalSupply({
blockNumber: proposal.blockNumber,
tokenAddress: proposal?.settings.tokenAddress,
network: proposal.network,
blockTimestamp: 0,
hasClockMode: false,
clockMode: token?.clockMode!,

Copilot AI Aug 3, 2025

Copy link

Choose a reason for hiding this comment

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

Using the non-null assertion operator (!) without proper null checking is risky. Consider handling the case where token?.clockMode might be undefined.

Copilot uses AI. Check for mistakes.
@@ -137,7 +137,6 @@ class BlockchainLogCrawler {
strategy: this.crawlParams.strategy,
toBlock,
latestBlock,

Copilot AI Aug 3, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Removing the logsLen property from logging may make debugging more difficult when troubleshooting crawler issues.

Suggested change
latestBlock,
latestBlock,
logsLen: 0,

Copilot uses AI. Check for mistakes.
cristianizzo added a commit that referenced this pull request Sep 2, 2025
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.

2 participants