Conversation
- Add StellarAddress type with sql.Scanner/driver.Valuer for automatic conversion between string addresses (G.../C...) and 33-byte BYTEA - Update accounts table: stellar_address TEXT -> BYTEA - Update transactions_accounts table: account_id TEXT -> BYTEA
- Use StellarAddress type for Get, Insert, Delete operations - Convert []string to [][]byte for BatchGetByIDs ANY() clause - Handle mixed BYTEA/VARCHAR in IsAccountFeeBumpEligible query
- Convert addresses to []byte for BatchInsert UNNEST($11::bytea[]) - Use raw []byte instead of pgtype.Text for BatchCopy
- Pass StellarAddress type which implements driver.Valuer for auto-conversion
- Test nil, valid, and error cases for both Scan and Value - Test roundtrip conversion preserves address
- Fix IsAccountFeeBumpEligible to use separate EXISTS checks instead of UNION (avoids type mismatch between BYTEA and VARCHAR columns) - Update test fixtures to use types.StellarAddress for BYTEA columns - Use types.StellarAddress for scanning account_id from transactions_accounts
This is the first step in converting the operations_accounts table to use BYTEA storage for account IDs, matching the pattern already used by transactions_accounts.
All account_id columns now use BYTEA storage format, so the string case handling for TEXT columns is no longer needed.
All account_id columns now use BYTEA format, so the conditional conversion logic is no longer needed. Always use AddressBytea.
Both operations_accounts and transactions_accounts now use BYTEA for account_id, so the field is no longer needed in the config.
Convert addresses to BYTEA format using AddressBytea.Value() when copying to operations_accounts, matching the transactions pattern.
Convert accountAddr to BYTEA format in HasOperationForAccount query since operations_accounts.account_id is now BYTEA.
- operations_test.go: Use AddressBytea type for scanning account_id - accounts_test.go: Convert address to BYTEA in INSERT statement - test_utils.go: Use AddressBytea directly (already has Value() method)
- Change SQL query to use UNNEST($9::bytea[]) instead of text - Convert addresses to []byte using AddressBytea.Value()
- Use AddressBytea type for scanning account_id in both BatchInsert and BatchCopy test verification - Use AddressBytea when inserting test accounts
Update migration to use BYTEA type for 7 account_id columns: - account_id - signer_account_id, spender_account_id - sponsored_account_id, sponsor_account_id - deployer_account_id, funder_account_id
Helper function to convert nullable address strings to bytes for BYTEA insert operations. Returns nil for invalid/empty addresses.
Update the StateChange struct to use AddressBytea type for account_id field to match the BYTEA database column type.
Convert 7 account_id columns from text arrays to bytea arrays: - account_id (required) uses AddressBytea.Value() - 6 nullable columns use pgtypeBytesFromNullStringAddress helper
Convert 7 account_id columns from pgtype.Text to raw []byte: - account_id (required) uses AddressBytea.Value() - 6 nullable columns use pgtypeBytesFromNullStringAddress helper
Convert accountAddress parameter to AddressBytea for BYTEA column query.
aditya1702
changed the title
accounts, transactions_accounts: Store account_id as BYTEAaccount_id fields as BYTEA
There was a problem hiding this comment.
Pull request overview
This pull request converts storage of account_id fields from TEXT to BYTEA in the database. The change optimizes storage and query performance for Stellar addresses by storing them in their native 33-byte binary format (1 version byte + 32 raw key bytes) rather than as 56-character StrKey strings.
Changes:
- Introduced
AddressByteaandNullAddressByteatypes with SQL Scanner/Valuer interfaces for encoding/decoding between StrKey strings and BYTEA storage - Updated database migrations to change
account_idcolumns from TEXT to BYTEA in accounts, transactions_accounts, operations_accounts, and state_changes tables - Modified data access layer to convert addresses to/from BYTEA when inserting or querying
- Updated tests to use valid Stellar addresses generated from keypairs instead of hardcoded placeholder strings
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/indexer/types/types.go | Added AddressBytea and NullAddressBytea types with Scan/Value implementations for BYTEA conversion |
| internal/indexer/types/types_test.go | Added comprehensive tests for AddressBytea Scan/Value/String methods and roundtrip conversion |
| internal/db/migrations/*.sql | Updated CREATE TABLE statements to use BYTEA for all account_id columns |
| internal/data/accounts.go | Updated queries to convert string addresses to BYTEA; refactored IsAccountFeeBumpEligible to use OR instead of UNION |
| internal/data/transactions.go | Added address-to-BYTEA conversion for transactions_accounts link table insertions |
| internal/data/operations.go | Added address-to-BYTEA conversion for operations_accounts link table insertions |
| internal/data/statechanges.go | Added conversion for account_id and nullable address fields to BYTEA |
| internal/data/query_utils.go | Added pgtypeBytesFromNullAddressBytea helper for nullable address conversions |
| internal/utils/sql.go | Added NullAddressBytea helper function for creating nullable address values |
| internal/services/ingest_test.go | Replaced hardcoded test addresses with valid addresses from keypairs |
| internal/serve/graphql/resolvers/*.go | Updated resolvers to convert AddressBytea to string for GraphQL responses |
| internal/indexer/*.go | Updated to handle AddressBytea type conversions |
| internal/integrationtests/infrastructure/backfill_helpers.go | Updated queries to use AddressBytea for account address parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Test addresses generated from valid keypairs for use in tests. | ||
| // These are deterministic seeds to ensure consistent test addresses. | ||
| var ( | ||
| testKP1 = keypair.MustRandom() | ||
| testKP2 = keypair.MustRandom() | ||
| testKP3 = keypair.MustRandom() | ||
| testAddr1 = testKP1.Address() | ||
| testAddr2 = testKP2.Address() | ||
| testAddrUnreg = testKP3.Address() |
There was a problem hiding this comment.
The comment says "These are deterministic seeds to ensure consistent test addresses" but keypair.MustRandom() generates random keypairs, not deterministic ones. This means test addresses will be different on each test run, which could affect test reproducibility. Consider using keypair.MustParseFull() with fixed seed strings if deterministic addresses are needed, or update the comment to accurately reflect that these are randomly generated.
| // sharedTestAccountAddress is a fixed test address used by tests that rely on setupDB. | ||
| // It's generated once and reused to ensure test data consistency. | ||
| var sharedTestAccountAddress = keypair.MustRandom().Address() |
There was a problem hiding this comment.
The comment says "It's generated once and reused to ensure test data consistency" but keypair.MustRandom() generates a random keypair. While it's generated once per test run (as a package-level variable), it will be different between test runs. Consider using a fixed test address if true consistency across runs is needed, or clarify that the comment refers to consistency within a single test run.
| if err != nil { | ||
| return nil, fmt.Errorf("converting account_id: %w", err) | ||
| } | ||
| accountIDBytes[i] = addrBytes.([]byte) |
There was a problem hiding this comment.
Potential panic if addrBytes is nil. While the AddressBytea.Value() method returns nil for empty strings, the code unconditionally performs a type assertion addrBytes.([]byte) without checking if addrBytes is nil first. For required account_id fields, this should not happen in practice since valid state changes should have non-empty account IDs. However, consider adding a nil check or validation to make the code more defensive, or add a comment explaining why this case is impossible.
| accountIDBytes[i] = addrBytes.([]byte) | |
| if addrBytes == nil { | |
| return nil, fmt.Errorf("converting account_id: got nil BYTEA for required field") | |
| } | |
| accountIDByteSlice, ok := addrBytes.([]byte) | |
| if !ok { | |
| return nil, fmt.Errorf("converting account_id: unexpected type %T, want []byte", addrBytes) | |
| } | |
| accountIDBytes[i] = accountIDByteSlice |
| if err != nil { | ||
| return nil, fmt.Errorf("converting address %s to bytes: %w", address, err) | ||
| } | ||
| stellarAddressBytes = append(stellarAddressBytes, addrBytes.([]byte)) |
There was a problem hiding this comment.
Potential panic if addrBytes is nil. The code performs an unconditional type assertion addrBytes.([]byte) without checking if addrBytes is nil. While AddressBytea.Value() only returns nil for empty strings (which shouldn't occur for valid addresses), consider adding a nil check or validation to make the code more defensive.
| stellarAddressBytes = append(stellarAddressBytes, addrBytes.([]byte)) | |
| if addrBytes == nil { | |
| return nil, fmt.Errorf("converting address %s to bytes: got nil value", address) | |
| } | |
| addrByteSlice, ok := addrBytes.([]byte) | |
| if !ok { | |
| return nil, fmt.Errorf("converting address %s to bytes: unexpected type %T", address, addrBytes) | |
| } | |
| stellarAddressBytes = append(stellarAddressBytes, addrByteSlice) |
| addrBytes, err := types.AddressBytea(address).Value() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("converting address %s to bytes: %w", address, err) | ||
| } | ||
| stellarAddressBytes = append(stellarAddressBytes, addrBytes.([]byte)) |
There was a problem hiding this comment.
Potential panic if addrBytes is nil. The code performs an unconditional type assertion addrBytes.([]byte) without checking if addrBytes is nil. While AddressBytea.Value() only returns nil for empty strings (which shouldn't occur for valid addresses), consider adding a nil check or validation to make the code more defensive.
| addrBytes, err := types.AddressBytea(address).Value() | |
| if err != nil { | |
| return nil, fmt.Errorf("converting address %s to bytes: %w", address, err) | |
| } | |
| stellarAddressBytes = append(stellarAddressBytes, addrBytes.([]byte)) | |
| addrBytesValue, err := types.AddressBytea(address).Value() | |
| if err != nil { | |
| return nil, fmt.Errorf("converting address %s to bytes: %w", address, err) | |
| } | |
| addrBytes, ok := addrBytesValue.([]byte) | |
| if !ok || addrBytes == nil { | |
| return nil, fmt.Errorf("converting address %s to bytes: unexpected value %T", address, addrBytesValue) | |
| } | |
| stellarAddressBytes = append(stellarAddressBytes, addrBytes) |
| if err != nil { | ||
| return 0, fmt.Errorf("converting address %s to bytes: %w", addr, err) | ||
| } | ||
| taRows = append(taRows, []any{toIDPgtype, addrBytes}) |
There was a problem hiding this comment.
Missing nil check before appending addrBytes. If AddressBytea.Value() returns nil for an empty address, this would append a nil value to taRows, which might work correctly for BYTEA columns. However, for clarity and to prevent potential issues, consider either validating that addresses are non-empty before conversion, or explicitly handling the nil case.
| taRows = append(taRows, []any{toIDPgtype, addrBytes}) | |
| if addrBytes == nil { | |
| taRows = append(taRows, []any{toIDPgtype, nil}) | |
| } else { | |
| taRows = append(taRows, []any{toIDPgtype, addrBytes}) | |
| } |
| @@ -1,7 +1,7 @@ | |||
| -- +migrate Up | |||
There was a problem hiding this comment.
The PR description contains unfilled TODO placeholders for "What", "Why", "Known limitations", and "Issue that this PR addresses". Please complete the PR description with the actual information before merging. This helps reviewers understand the context and future developers understand why these changes were made.
| } | ||
|
|
||
| // BatchGetByIDs returns the subset of provided account IDs that exist in the accounts table. | ||
| // BatchGetByIDs returns the subset of provided account IDs that exist in the accounts table. |
There was a problem hiding this comment.
Duplicate comment found. Line 112 duplicates line 113.
| // BatchGetByIDs returns the subset of provided account IDs that exist in the accounts table. |
| if err != nil { | ||
| return 0, fmt.Errorf("converting address %s to bytes: %w", addr, err) | ||
| } | ||
| oaRows = append(oaRows, []any{opIDPgtype, addrBytes}) |
There was a problem hiding this comment.
Missing nil check before appending addrBytes. If AddressBytea.Value() returns nil for an empty address, this would append a nil value to oaRows, which might work correctly for BYTEA columns. However, for clarity and to prevent potential issues, consider either validating that addresses are non-empty before conversion, or explicitly handling the nil case.
| oaRows = append(oaRows, []any{opIDPgtype, addrBytes}) | |
| if addrBytes == nil { | |
| oaRows = append(oaRows, []any{opIDPgtype, nil}) | |
| } else { | |
| oaRows = append(oaRows, []any{opIDPgtype, addrBytes}) | |
| } |
1 participant
What
[TODO: Short statement about what is changing.]
Why
[TODO: Why this change is being made. Include any context required to understand the why.]
Known limitations
[TODO or N/A]
Issue that this PR addresses
[TODO: Attach the link to the GitHub issue or task. Include the priority of the task here in addition to the link.]
Checklist
PR Structure
allif the changes are broad or impact many packages.Thoroughness
Release