-
Notifications
You must be signed in to change notification settings - Fork 14
refactor: rename cardano_address to cardano_stake_key for clarity #628
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
base: main
Are you sure you want to change the base?
refactor: rename cardano_address to cardano_stake_key for clarity #628
Conversation
This commit addresses issue midnightntwrk#440 by renaming the `cardano_address` column and field to `cardano_stake_key` throughout the codebase. The previous name was confusing because the field actually stores a Cardano stake key (reward address), not a regular Cardano address. Changes made: 1. Domain layer (chain-indexer/src/domain/dust.rs): - Renamed field in DustRegistrationEvent enum variants - Updated documentation comments 2. Infrastructure layer (chain-indexer/src/infra/storage.rs): - Updated SQL queries to use cardano_stake_key - Updated pattern matching for DustRegistrationEvent 3. Runtime layer (chain-indexer/src/infra/subxt_node/runtimes.rs): - Updated event construction to use cardano_stake_key field name 4. API storage layer (indexer-api/src/infra/storage/dust.rs): - Updated SQL query for registration lookup 5. Database migrations: - Updated initial migrations (001_initial.sql) for new installations - Added migration 003_rename_cardano_address.sql for existing databases - Both PostgreSQL and SQLite migrations included This is a breaking change for existing databases that requires running the new migration. The migration handles: - Column rename - Index recreation with new name - Unique constraint update Closes midnightntwrk#440
|
Manus AI seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Hi @hseeberger, thanks for the heads up. I agree with your point about the migration files - we only use the single 001_initial.sql approach for cloud and standalone modes. I'll also leave some review comments for @roman98Z. |
cosmir17
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roman98Z, thanks for the PR.
I have left review comments.
Hope you find them useful 🙏
btw, regarding your note in the PR description:
The code has been validated with:
cargo check
I don't think that's good enough.
just all is the command that we use.
it does linting check, fmt, running unit and integration tests etc.
| @@ -0,0 +1,43 @@ | |||
| -------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not create new migration files before mainnet launch (that's my understanding).
Instead, update the existing 001_initial.sql files only.
Please remove:
- indexer-common/migrations/postgres/003_rename_cardano_address.sql
- indexer-common/migrations/sqlite/003_rename_cardano_address.sql
Keep: Your changes to 001_initial.sql files are correct.
| -- so we need to recreate the table with the new column name. | ||
|
|
||
| -- Step 1: Create a new table with the correct column name | ||
| CREATE TABLE cnight_registrations_new ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every code in this file is not needed. they are redundant.
| RENAME COLUMN cardano_address TO cardano_stake_key; | ||
|
|
||
| -- Drop and recreate the index with the new column name | ||
| DROP INDEX IF EXISTS cnight_registrations_cardano_address_idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for migration, hence, we don't need operations and codes in this file.
|
After making the code changes, could you also update the PR description to remove references to the 003 migration files?
This keeps the description accurate, easy to read and aligned with our migration approach. |
Refactor: Rename cardano_address to cardano_stake_key for Clarity
Overview
This Pull Request addresses issue #440 by renaming the
cardano_addresscolumn and field tocardano_stake_keythroughout the codebase. The previous naming was confusing because the field actually stores a Cardano stake key (reward address), not a regular Cardano address.As noted in the issue: "the column name was what drove me off the road" - Giuseppe
Problem Statement
The
cardano_addressnaming was misleading because:CardanoRewardAddress, making the column name inconsistent with the domain modelSolution
Renamed all occurrences of
cardano_addresstocardano_stake_keyto accurately reflect the data being stored.Changes Summary
Rust Code Changes
chain-indexer/src/domain/dust.rsDustRegistrationEventenum variantschain-indexer/src/infra/storage.rschain-indexer/src/infra/subxt_node/runtimes.rsindexer-api/src/infra/storage/dust.rsDatabase Migration Changes
indexer-common/migrations/postgres/001_initial.sqlindexer-common/migrations/sqlite/001_initial.sqlindexer-common/migrations/postgres/003_rename_cardano_address.sqlindexer-common/migrations/sqlite/003_rename_cardano_address.sqlMigration Details
For New Installations
The initial migration (
001_initial.sql) has been updated to usecardano_stake_keydirectly, so new installations will have the correct schema from the start.For Existing Databases
A new migration (
003_rename_cardano_address.sql) handles the transition:PostgreSQL:
ALTER TABLE ... RENAME COLUMNSQLite:
Breaking Changes
This is a breaking change for existing databases. Users must run the new migration before deploying this version.
Testing
The code has been validated with:
Result: ✅ Compiles successfully with no warnings or errors
Related Issues
Checklist