Conversation
- Added 5 second timeout to /health and /health/all endpoints - If health check takes longer than 5s, returns unhealthy instead of hanging - Refactored handlers to use timeout utility from tokio::time - Fixed return type issue where Err variant had wrong format
…egator The std::sync::Mutex was causing the async tokio runtime to block when the health check tried to acquire the lock, leading to health check timeouts and Discord gateway disconnects. Changed to tokio::sync::Mutex which properly yields to the scheduler when contended.
- Fix periodic Discord connectivity test: was mathematically impossible (mod 4 vs mod 10) - Fix loop exit logic: return Err in async block didn't exit loop, use break instead - Fix fire-and-forget periodic test: now awaits and checks failures synchronously - Fix Mutex API usage: tokio::sync::Mutex returns guard directly, not Result - Add time-based connectivity check every 60 seconds - Add restart triggers: gateway_failures>=5, discord_test_failures>=3, no Discord success >2min - Remove unreachable!() panic in format_custom_status
Change % 4 to % 40 for update_count to allow periodic connectivity test to trigger, but update all match expressions to use update_count % 4 so they cycle properly instead of panicking on values 4-39
SQLite doesn't support DELETE ... LIMIT until version 3.35.0. Wrap LIMIT in subquery to delete by rowid instead.
…bots - Increased busy_timeout to 60s - Increased pool size from 4 to 16 connections - Added min_idle of 4 connections - Added 64MB cache - Added temp_store = MEMORY Note: PostgreSQL migration attempted but requires async rewrite of all call sites
- Replace rusqlite/r2d2 with async sqlx and PgPool - All database methods are now async fn - Add PostgreSQL service to docker-compose.yml with healthcheck - DATABASE_URL env var replaces DATABASE_PATH - Update all code to use .await for database calls - PostgreSQL eliminates 'database is locked' errors with 24+ bots
Issue 4 - DB Write Failures: - Add db_failures counter to HealthState - Increment on DB write failure, reset on success - Mark unhealthy if db_failures > 3 Issue 5 - Health Server Bind Failure: - Add start_health_server_with_retry() with 10 retries - Health server bind failure is now non-fatal - Bots continue running even if health server fails Issues 1 & 6 - Silent Task Death and Supervision: - All spawned tasks now have supervision loops - Services auto-restart on unexpected exit - Store and monitor all service join handles - Log CRITICAL errors when services die unexpectedly Issue 2 - Race Condition on prices.json: - Add SharedPrices with tokio::sync::RwLock - Price service writes to shared state atomically first - Bots read from shared state instead of file - File write happens AFTER atomic state update
- Fix syntax error in database.rs (extra parenthesis) - Fix Arc/SharedPrices type mismatches across modules - Fix i64 vs u64 type casts in db_cleanup.rs - Fix duplicate Arc import in price_service.rs - Fix private struct re-exports from price_state - Fix ticker move before use in async spawn loop - Fix health_server return type in match arm
- Python bot using discord.py (more stable Discord ecosystem) - PostgreSQL via asyncpg (native async, connection pooling) - Pyth Network API for price feeds - Simple docker-compose with build inside - Environment-based configuration for bot tokens
- Use asyncio.run() to properly run async tasks - Catch table/index creation errors gracefully - Allow multiple bots to initialize DB concurrently
- Status cycles through BTC, ETH, SOL every 30 seconds - Shows 1h percentage change from database - Nickname shows crypto and formatted price
- Shanghai Silver website is JS-rendered, scrape doesn't work - Only update SHANGHAISILVER if price > 10 (silver is ~30+) - Otherwise use cached database price
…available - Site uses client-side JavaScript rendering, cannot scrape - Rust version may have worked with different site configuration - SHANGHAISILVER now shows no price until alternative source found - Other cryptos (BTC, ETH, SOL, etc.) work fine with Pyth Network
- Fixed extraction to find dollar amounts after 'Shanghai Spot' - Pattern: find prefix, then look for $ followed by number - Shanghai Silver now fetches ~$92 from goldsilver.ai
- Added Yahoo Finance API support for DXY - Uses chart API endpoint with regularMarketPrice - DXY now shows ~98 in Discord
- Nickname: CRYPTO - Status cycles: BTC value (e.g., ₿0.024421) then 1h change, then BTC, etc. - For BTC bot, only shows 1h percentage (no self-conversion)
- Status cycles: BTC value, ETH value, SOL value, 1h%, then repeat - Each crypto shows its value in terms of the other three - For BTC, shows ETH, SOL, 1h% (no self-conversion) - For ETH, shows BTC, SOL, 1h% (no self-conversion) - Same for SOL
- Clean documentation without emojis or special characters - Updated architecture diagram for PostgreSQL - Removed Rust/SQLite/Plotters references - Added DXY, SHANGHAISILVER, GOLD/SILVER sources - Document status cycling behavior - Added project structure section
- Added chart_service.py with matplotlib for beautiful dark-themed charts - Added /chart price command showing 24h/7d/30d price charts with high/low markers - Added /price current command with USD price, 24h/7d/30d percentage changes, and BTC/ETH/SOL conversions - Changed SHANGHAISILVER ticker to SSILVER for brevity - Added font dependencies to Dockerfile for matplotlib - Updated README with slash commands documentation - Chart commands scoped per-bot (BTC bot shows BTC charts only, etc.)
![gitzillabot[bot]](https://avatars.githubusercontent.com/in/2996112?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gitzilla has reviewed your changes and found 5 potential issues.
Autofix is OFF. To automatically fix reported issues, enable autofix in the Gitzilla dashboard.
| # Format: CRYPTO:feed_id,CRYPTO:feed_id,... | ||
| CRYPTO_FEEDS=BTC:0xe62df6c8b4a85fe1a67db44dc12de5db330f7ac66b72dc658afedf0f4a415b43,ETH:0xff61491a931112ddf1bd8147cd1b641375f79f5825126d665480874634fd0ace,SOL:0xef0d8b6fda2ceba41da15d4095d1da392a0d2f8ed0c6c7bc0f4cfac8c280b56d | ||
|
|
There was a problem hiding this comment.
Hardcoded database credentials committed to repository
Critical Severity
The .env.example file contains hardcoded PostgreSQL credentials: POSTGRES_PASSWORD=PdefSMMIa8N22nKwHxmWz5znC13bUFo and DATABASE_URL=postgresql://postgres:PdefSMMIa8N22nKwHxmWz5znC13bUFo@postgres:5432/pricebot. The .gitignore only ignores .env, .env.local, .env.production but NOT .env.example, so these credentials will be committed to version control. Anyone with repository access can extract these credentials and gain database access. This is a severe security vulnerability that could lead to unauthorized database access and potential data breaches.
Suggested fix: Remove the hardcoded password values from .env.example. Replace with placeholder values that clearly indicate they must be changed: POSTGRES_PASSWORD=changeme and DATABASE_URL=postgresql://postgres:changeme@postgres:5432/pricebot. The existing comment "CHANGE THESE PASSWORDS" is insufficient as users may miss it or copy-paste without modification.
| @@ -0,0 +1,148 @@ | |||
| """ | |||
There was a problem hiding this comment.
No database data retention policy — unbounded table growth
High Severity
The Rust implementation had a db_cleanup.rs module that ran periodic cleanup, deleting old price data based on retention tiers (24 hours raw, 7 days minute-level, etc.). The Python rewrite completely removed this cleanup logic. The price_aggregates table exists in _create_tables() but is never populated, and there's no scheduled task to purge old records. The prices table will grow indefinitely with no cleanup, leading to degraded query performance over time and unbounded disk usage.
Suggested fix: Add a cleanup task that periodically removes old price records. For example, run a daily task that deletes prices older than 30 days (or whatever retention period is appropriate): DELETE FROM prices WHERE timestamp < $1 where $1 = time.time() - 30243600.
| """, crypto_name.upper(), price, timestamp) | ||
|
|
||
| return True | ||
|
|
||
| async def get_latest_price(self, crypto_name: str) -> Optional[float]: | ||
| """Get the latest price for a cryptocurrency.""" | ||
| async with self.pool.acquire() as conn: | ||
| row = await conn.fetchrow(""" | ||
| SELECT price FROM prices | ||
| WHERE crypto_name = $1 | ||
| ORDER BY timestamp DESC | ||
| LIMIT 1 | ||
| """, crypto_name.upper()) | ||
|
|
There was a problem hiding this comment.
get_price_history has no row limit — potential memory exhaustion
Medium Severity
In database.py, the get_price_history() method queries the database with only a time cutoff filter and no LIMIT clause. For long timeframes like 3m (2160 hours), this could return thousands of rows. These rows are loaded into a list and then used to generate matplotlib charts, potentially causing high memory usage. Combined with the lack of cleanup (previous issue), this method will progressively return more data over time.
Suggested fix: Add a reasonable row limit (e.g., 1000-2000 points for charts) using a subquery or ORDER BY/LIMIT clause to cap memory usage.
| edgecolor='none', | ||
| dpi=100 | ||
| ) | ||
| buf.seek(0) | ||
| plt.close(fig) | ||
|
|
||
| return buf.read() | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to generate chart for {crypto_name}: {e}") | ||
| return None | ||
|
|
||
| async def get_chart_bytes( | ||
| self, |
There was a problem hiding this comment.
get_chart_bytes passes raw DB history to chart generator without aggregation
Medium Severity
In chart_service.py, get_chart_bytes() retrieves raw price history from the database (up to potentially thousands of rows per timeframe) and passes it directly to generate_price_chart(). This defeats the purpose of having the price_aggregates table (which exists but is never used). For long timeframes, matplotlib will try to plot every single data point, creating cluttered charts and high memory/CPU usage.
Suggested fix: Use the price_aggregates table for longer timeframes, or implement simple downsampling in get_chart_bytes (e.g., for >24h charts, use every Nth point or aggregate to hourly/daily candles).
| if crypto == "SSILVER" and (price is None or price < 10): | ||
| db_price = await self.db.get_latest_price(crypto) | ||
| if db_price and db_price > 10: | ||
| logger.debug(f"SSILVER: Using cached price ${db_price}") | ||
| return db_price | ||
| logger.warning(f"SSILVER: No valid price (got {price}), skipping update") | ||
| return None | ||
|
|
||
| if price is None or price <= 0: | ||
| db_price = await self.db.get_latest_price(crypto) | ||
| if db_price and db_price > 0: | ||
| logger.debug(f"Using cached {crypto} price: ${db_price}") | ||
| return db_price | ||
| return None | ||
| return price | ||
|
|
||
| async def get_conversion_prices(self) -> dict: | ||
| """Get BTC, ETH, SOL prices for conversion.""" | ||
| prices = {} | ||
| for ticker in ["BTC", "ETH", "SOL"]: | ||
| try: | ||
| p = await self.price_service.get_price(ticker) | ||
| if p and p > 0: | ||
| prices[ticker] = p | ||
| else: | ||
| db_p = await self.db.get_latest_price(ticker) | ||
| if db_p and db_p > 0: | ||
| prices[ticker] = db_p | ||
| except Exception as e: | ||
| logger.debug(f"Could not get {ticker} price: {e}") | ||
| return prices |
There was a problem hiding this comment.
Real-time price updates not saved to database
Low Severity
In bot.py, the start_price_updates() method calls get_price_for_crypto() and update_discord_presence() in a loop, but only saves prices to the database when price is valid. However, conversion prices for BTC/ETH/SOL (fetched in get_conversion_prices()) are never persisted to the database. These prices are only stored when users invoke the /price current command. This means conversion rates will be stale on bot restart and unavailable during gaps between user commands.
Suggested fix: Save BTC, ETH, SOL prices to the database in the price update loop alongside the primary crypto price, using the same await self.db.save_price() calls.
There was a problem hiding this comment.
Gitzilla has reviewed your changes and found 4 potential issues.
Autofix is OFF. To automatically fix reported issues, enable autofix in the Gitzilla dashboard.
| @@ -1,41 +1,17 @@ | |||
| # Price update interval (shared by all services) | |||
| UPDATE_INTERVAL_SECONDS=30 | |||
| # Price update interval in seconds | |||
There was a problem hiding this comment.
Hardcoded database credentials in .env.example
Critical Severity
The .env.example file contains what appears to be a real database password: POSTGRES_PASSWORD=PdefSMMIa8N22nKwHxmWz5znC13bUFo and a corresponding DATABASE_URL with the same password embedded. Example/template files should contain only placeholder values like your_password_here, never actual credentials. If users copy this file without editing, they may unknowingly use these credentials in production. Anyone with read access to the repository can obtain these credentials.
Suggested fix: Replace PdefSMMIa8N22nKwHxmWz5znC13bUFo with placeholder text like your_secure_password_here in both POSTGRES_PASSWORD and DATABASE_URL.
| @@ -0,0 +1,149 @@ | |||
| """ | |||
There was a problem hiding this comment.
Unbounded database table growth with no retention policy
Medium Severity
The prices table in database.py has no data retention or cleanup mechanism. Prices are inserted every UPDATE_INTERVAL_SECONDS (default 12 seconds) for each tracked cryptocurrency. Without any DELETE or archival policy, the table will grow indefinitely. Over time, this will degrade query performance and consume increasing storage. The Rust version had a DatabaseCleanup service that aggregated and compacted old data - this functionality is missing from the Python rewrite.
Suggested fix: Add a periodic cleanup task or PostgreSQL policy to delete or archive price records older than a configurable retention period (e.g., 30-90 days).
| # SSILVER: only use if it looks valid (silver is ~$30+, so > 10) | ||
| if crypto == "SSILVER" and (price is None or price < 10): | ||
| db_price = await self.db.get_latest_price(crypto) | ||
| if db_price and db_price > 10: | ||
| logger.debug(f"SSILVER: Using cached price ${db_price}") | ||
| return db_price | ||
| logger.warning(f"SSILVER: No valid price (got {price}), skipping update") | ||
| return None |
There was a problem hiding this comment.
SSILVER price updates skipped when live price is below validation threshold
Medium Severity
In bot.py get_price_for_crypto, when SSILVER's live price from goldsilver.ai is below the $10 validation threshold, the function returns None instead of falling back to the cached database price. Unlike other cryptocurrencies (lines 135-140) which correctly fall back to cached prices when live price is invalid, SSILVER (lines 127-133) skips the entire update when its price is between 0-10. This means SSILVER will fail silently during periods when the API returns temporarily low values, instead of using the last known good price from the database.
Suggested fix: Apply the same fallback pattern used for other cryptocurrencies to SSILVER: when live price is invalid (None or < 10), check for a cached price in the database before returning None.
| crypto_name TEXT NOT NULL, | ||
| price REAL NOT NULL, | ||
| timestamp BIGINT NOT NULL, | ||
| created_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP | ||
| ) |
There was a problem hiding this comment.
Database schema uses REAL type for price storage
Medium Severity
The prices table schema uses REAL type (4-byte floating point with ~7 significant digits) for the price column. This can cause cumulative precision loss for financial data, especially problematic when storing high-frequency price updates (every 12 seconds) and computing percentage changes. PostgreSQL's DOUBLE PRECISION (8 bytes, ~15 significant digits) or NUMERIC would preserve price accuracy better.
Suggested fix: Change price REAL NOT NULL to price DOUBLE PRECISION NOT NULL in the CREATE TABLE statement for better precision.
- Raw data: last 24h - 5-min aggregates: 7 days - Hourly aggregates: 30 days - Daily aggregates: 1 year - Weekly aggregates: 5 years - Older data: auto-deleted Chart queries auto-select appropriate aggregation level. Maintenance runs hourly, cleanup runs daily.
There was a problem hiding this comment.
Gitzilla has reviewed your changes and found 1 potential issue.
Autofix is OFF. To automatically fix reported issues, enable autofix in the Gitzilla dashboard.
| # Database - CHANGE THESE PASSWORDS | ||
| POSTGRES_USER=postgres | ||
| POSTGRES_PASSWORD=PdefSMMIa8N22nKwHxmWz5znC13bUFo | ||
| DATABASE_URL=postgresql://postgres:PdefSMMIa8N22nKwHxmWz5znC13bUFo@postgres:5432/pricebot |
There was a problem hiding this comment.
Hardcoded database password in .env.example
High Severity
The .env.example file contains a hardcoded database password (PdefSMMIa8N22nKwHxmWz5znC13bUFo) instead of a placeholder value. While the comment says "CHANGE THESE PASSWORDS", having actual credentials in the repository creates a security risk - the password will remain in git history and users may deploy with the real credential.
Suggested fix: Replace the hardcoded password with a placeholder like your_secure_password_here and update the comment to make it clear this must be changed before deployment.
There was a problem hiding this comment.
Gitzilla has reviewed your changes and found 6 potential issues.
Autofix is OFF. To automatically fix reported issues, enable autofix in the Gitzilla dashboard.
| except Exception as e: | ||
| logger.warning(f"Index creation warning (may be OK): {e}") | ||
|
|
||
| logger.info("Database tables initialized") | ||
|
|
||
| async def _should_run_task(self, last_run: float, interval: int) -> bool: | ||
| """Check if a task should run based on interval.""" | ||
| return (time.time() - last_run) >= interval | ||
|
|
||
| async def _aggregate_prices(self): | ||
| """Aggregate raw prices into time buckets.""" | ||
| now = int(time.time()) | ||
|
|
||
| buckets = [ | ||
| (300, 7 * ONE_DAY), # 5-min aggregates for 7 days | ||
| (ONE_HOUR, 30 * ONE_DAY), # hourly aggregates for 30 days | ||
| (ONE_DAY, 365 * ONE_DAY), # daily aggregates for 1 year | ||
| (ONE_WEEK, 5 * 365 * ONE_DAY), # weekly for 5 years | ||
| ] | ||
|
|
There was a problem hiding this comment.
Price aggregation query uses wrong time window
High Severity
The _aggregate_prices method in database.py has a flawed time window calculation. The query WHERE timestamp >= $3 AND timestamp < $1 with $1=bucket_start looks for prices BEFORE bucket_start (e.g., prices before 10:40 when bucket_start=10:40), but at runtime the current time is AFTER bucket_start (e.g., 10:42). This means the aggregation either captures stale data or nothing at all. For 5-minute buckets, this should aggregate the PREVIOUS 5-minute window (10:35-10:40), not all data up to bucket_start. The correct WHERE clause should be timestamp >= $1 - $2 AND timestamp < $1.
Suggested fix: Change the WHERE clause to: WHERE timestamp >= $1 - $2 AND timestamp < $1 to capture the correct bucket period (the time window BEFORE bucket_start, not everything up to it).
| if self._should_run_task(self._last_aggregate, ONE_HOUR): | ||
| await self._aggregate_prices() | ||
| self._last_aggregate = now |
There was a problem hiding this comment.
Fire-and-forget maintenance tasks can race
Medium Severity
In save_price, maintenance is spawned with asyncio.create_task(self._run_maintenance()) without tracking or awaiting the task. If prices are saved rapidly (e.g., multiple bot instances updating), multiple maintenance tasks can execute concurrently because _should_run_task only checks time elapsed, not whether a task is already running. There's no locking mechanism to prevent overlapping aggregate/cleanup operations.
Suggested fix: Add an asyncio.Lock to protect the maintenance task, e.g., self._maintenance_lock = asyncio.Lock() and check/acquire it before running maintenance tasks.
| FROM price_aggregates | ||
| WHERE crypto_name = $1 AND bucket_start > $2 AND bucket_duration = 86400 | ||
| ORDER BY bucket_start ASC | ||
| """) | ||
| elif hours <= 43800: | ||
| return ("weekly", """ | ||
| SELECT bucket_start as timestamp, avg_price as price | ||
| FROM price_aggregates | ||
| WHERE crypto_name = $1 AND bucket_start > $2 AND bucket_duration = 604800 | ||
| ORDER BY bucket_start ASC | ||
| """) | ||
| else: | ||
| return ("monthly", """ | ||
| SELECT bucket_start as timestamp, avg_price as price | ||
| FROM price_aggregates | ||
| WHERE crypto_name = $1 AND bucket_start > $2 AND bucket_duration = 2592000 | ||
| ORDER BY bucket_start ASC | ||
| """) |
There was a problem hiding this comment.
Unnecessary parameter passed to fallback query
Low Severity
In get_price_history, when the primary query returns no results and falls back to raw data, the query is rebuilt but still passes the limit parameter to conn.fetch. While asyncpg tolerates extra unused parameters, this is unnecessary since the fallback query doesn't have a LIMIT clause (it's only used in the non-fallback path).
Suggested fix: Pass only 2 arguments to the fallback query: conn.fetch(query, crypto_name.upper(), cutoff) without the limit parameter.
| @@ -1,41 +1,17 @@ | |||
| # Price update interval (shared by all services) | |||
| UPDATE_INTERVAL_SECONDS=30 | |||
| # Price update interval in seconds | |||
There was a problem hiding this comment.
Hardcoded database password committed to repository
High Severity
The .env.example file contains a hardcoded database password (PdefSMMIa8N22nKwHxmWz5znC13bUFo). While this is an example file, committing credentials to version control creates security risk if developers copy it without generating new credentials. Passwords should only be documented as placeholders or sourced from CI secrets.
Suggested fix: Remove the hardcoded password from .env.example and use a placeholder value like your_secure_password_here, or document that users must generate their own.
| def calculate_change_percent(current: float, previous: float) -> float: | ||
| """Calculate percentage change.""" | ||
| if previous <= 0: | ||
| return 0.0 | ||
| return ((current - previous) / previous) * 100 |
There was a problem hiding this comment.
ChartService instantiated but never used
Medium Severity
In bot.py, a ChartService is created and passed to PriceBot.init, where it's stored as self.chart_service. However, self.chart_service is never accessed anywhere in the PriceBot class. The ChartGroup in setup_hook receives its own chart_service parameter, making the instance variable dead code that consumes memory for no purpose.
Suggested fix: Either remove the chart_service parameter from PriceBot.init and the instantiation in run_bot, or wire it up properly if chart functionality was intended to be shared.
|
|
||
| async def get_chart_bytes( | ||
| self, | ||
| db, | ||
| crypto: str, | ||
| hours: int = 24, | ||
| timeframe_str: str = None | ||
| ) -> Optional[bytes]: |
There was a problem hiding this comment.
Unused _downsample method in ChartService
Low Severity
The _downsample method in chart_service.py is defined but never called. The get_chart_bytes method fetches data directly from the database with a limit parameter, bypassing the downsampling logic entirely.
Suggested fix: Either use the _downsample method to limit data points before chart generation, or remove it to reduce dead code.
1 participant
Complete rewrite from Rust to Python with new features: slash commands with charts, stable Python/discord.py, security fixes, CI/CD.
Note
High Risk
Overview
This pull request performs a complete rewrite from Rust to Python, replacing all Rust source files, configuration, and build tooling with Python equivalents. The new Python implementation includes
bot.pyas the main application along withprice_service.py,chart_service.py, anddatabase.pyfor modular functionality. The rewrite introduces slash commands with integrated charting capabilities and adopts discord.py for Discord integration. Rust-related files includingCargo.toml,Cargo.lock,rustfmt.toml, and allsrc/*.rsfiles have been removed.Written by Gitzilla for commit 3f769a2. This will update automatically on new runs. Configure in the Gitzilla dashboard.