Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node: Allow tokens to change their symbol in generator script #3968

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

djb15
Copy link
Collaborator

@djb15 djb15 commented Jun 6, 2024

When trying to update the governor token list, the script was failing on this check:

// Sanity check to make sure the script is doing what we think it is
    if (existingTokenKeys.length + addedTokens.length - removedTokens.length != newTokensCount) {
      console.error(`Num existing tokens (${existingTokenKeys.length}) + Added tokens (${addedTokens.length}) - Removed tokens (${removedTokens.length}) != Num new tokens (${newTokensCount})`);
      process.exit(1);
    }

It turned out that a token was being classified as "removed" because its symbol had changed. We should allow tokens to change their symbol (like we do on the new token side) so this PR updates the script to only use the chain + address key for removed token lookups; that's a unique identifier after all.

Even if the token symbol included a "-" character this change is safe since we only concat the first 2 elements of the array, and the symbol is the last element in the key strings.

@evan-gray evan-gray requested review from SEJeff and johnsaigle June 6, 2024 14:15
Copy link
Contributor

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

The change conceptually makes sense and I agree that it is still a unique identifier.
I ran the script locally and didn't encounter any issues.

I tried following up with go run check_query.go but I think I am rate-limited so that didn't work for me. Probably just something on my side though.

@djb15 djb15 merged commit 215c60a into main Jun 6, 2024
24 checks passed
@djb15 djb15 deleted the node/fix-token-list-script branch June 6, 2024 14:41
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.

4 participants