Skip to content

Refactor/op icon + number formatting for notification system#100

Merged
LeonardoVieira1630 merged 4 commits into
devfrom
refactor/OP-Icon-+-Number-Formatting-for-Notification-System
Jul 25, 2025
Merged

Refactor/op icon + number formatting for notification system#100
LeonardoVieira1630 merged 4 commits into
devfrom
refactor/OP-Icon-+-Number-Formatting-for-Notification-System

Conversation

@LeonardoVieira1630
Copy link
Copy Markdown
Member

Context

This PR introduces two small but impactful improvements to enhance the notification system's user experience.

First, it adds the OP DAO icon to the Telegram bot's DAO emoji mapping. Previously, OP-related notifications would display with the generic DAO icon, but now they'll show the distinctive red circle emoji that's associated with Optimism.

Second, it implements human-readable number formatting for voting power change notifications. The system was previously displaying raw token amounts in wei format, which meant users would see confusing numbers like "100000000000000000" instead of "0.1". The new formatting converts these 18-decimal values into friendly notation, showing "1.2K" instead of "1234" or "<0.1" for very small amounts. This makes it immediately clear to users how much voting power they've gained or lost.

The implementation includes comprehensive test coverage to ensure the formatting handles edge cases correctly, from tiny wei amounts to billions of tokens.

@LeonardoVieira1630 LeonardoVieira1630 self-assigned this Jul 21, 2025
@LeonardoVieira1630 LeonardoVieira1630 added the enhancement New feature or request label Jul 21, 2025
@LeonardoVieira1630 LeonardoVieira1630 marked this pull request as ready for review July 21, 2025 16:53
expect(formatTokenAmount('1000000000000000000000')).toBe('1.0K'); // 1,000 tokens
expect(formatTokenAmount('1234000000000000000000')).toBe('1.2K'); // 1,234 tokens
expect(formatTokenAmount('12345000000000000000000')).toBe('12.3K'); // 12,345 tokens
expect(formatTokenAmount('999999000000000000000000')).toBe('1000.0K'); // 999,999 tokens
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why 1000.0K and not 1M?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point

it('should format millions with M suffix', () => {
expect(formatTokenAmount('1000000000000000000000000')).toBe('1.0M'); // 1 million tokens
expect(formatTokenAmount('1234567000000000000000000')).toBe('1.2M'); // 1.234567 million tokens
expect(formatTokenAmount('999999999000000000000000000')).toBe('1000.0M'); // ~1 billion tokens
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here.

It's good to have these numbers, but it's rare to find tokens with more supply than 1B. Imagine seeing a delegation near to that, almost impossible.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I totally agree with you — that’s a great point. Because of that, I’ll remove the “Trillion” unit from the code.
However, I don’t think we should remove the “Billion” option. “Almost impossible” still means possible uheuheuheu.
But jokes aside, from a technical perspective, supporting “B” adds no real complexity — it’s just a matter of including 'B' in the array: const units = ['', 'K', 'M', 'B']. So keeping it improves robustness without adding overhead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, btw, this function isn’t limited to votingPower changes. As noted at the top of the file, it’s meant to be a general utility for formatting numbers in a human-readable way. So restricting it based on token decimals could make it less useful for its purpose.

Comment thread apps/dispatcher/src/lib/number-formatter.test.ts
Comment thread apps/dispatcher/src/lib/number-formatter.ts Outdated
Copy link
Copy Markdown
Member

@alextnetto alextnetto left a comment

Choose a reason for hiding this comment

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

lgtm!

@LeonardoVieira1630 LeonardoVieira1630 merged commit ea8ea74 into dev Jul 25, 2025
@LeonardoVieira1630 LeonardoVieira1630 deleted the refactor/OP-Icon-+-Number-Formatting-for-Notification-System branch July 25, 2025 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants