feat: track Handshake domain resource updates #455
Conversation
📝 WalkthroughWalkthroughAdds conversion of Handshake covenant resource data into DNS domain records (DS, NS, A/AAAA glue and synth, TXT) and integrates that conversion into RegisterCovenant and UpdateCovenant processing, persisting results via a new UpdateHandshakeDomain call. Expands state layer with separate key prefixes for Cardano and Handshake records and domains, and adds public helpers to update and lookup handshake-specific records (UpdateHandshakeDomain, LookupHandshakeRecords) plus generic wrappers (UpdateDomain, LookupRecords) that delegate to internal helpers managing write/delete and lookups with provided prefixes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Files likely needing extra attention:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/indexer/handshake.go(2 hunks)internal/state/state.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/indexer/handshake.go (3)
internal/state/state.go (1)
DomainRecord(43-48)internal/handshake/covenant.go (2)
UpdateCovenant(366-370)UpdateCovenant(372-372)internal/handshake/domain.go (1)
DomainResourceData(55-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/state/state.go (4)
31-35: LGTM! Good separation of concerns.The key prefix constants provide clear namespacing for Cardano and Handshake domain/record storage, preventing collisions and enabling independent management of each domain source.
223-233: LGTM! Clean refactoring with proper record lifecycle management.The delegation pattern allows code reuse between Cardano and Handshake domains while maintaining separate storage namespaces. The
updateDomainhelper properly handles record addition, tracking, and cleanup of obsolete records within a single transaction.Also applies to: 235-304
306-315: LGTM! Consistent API design for both domain sources.The lookup and update methods for Handshake domains mirror the Cardano equivalents, using the same underlying helpers with different key prefixes. The iterator-based lookup properly handles prefix matching and gob decoding.
Also applies to: 317-360, 398-419
284-284: > Likely an incorrect or invalid review comment.internal/indexer/handshake.go (4)
10-10: LGTM! Appropriate imports for DNS record handling.The
encoding/base32import supports synthetic NS name generation, andgithub.com/miekg/dnsprovides canonical name formatting for DNS records.Also applies to: 18-18
132-138: LGTM! Proper integration of domain record persistence.The covenant handlers correctly convert handshake resource data to domain records and persist them via
UpdateHandshakeDomain. Error propagation is appropriate.Also applies to: 145-151
283-285: LGTM! Good defensive error handling.Returning an error for unsupported record types prevents silent failures and makes debugging easier.
267-282: No issues with TXT record format.The TXT record handling at lines 267-282 correctly formats multiple items as space-separated quoted strings (
"item1" "item2" "item3"), which is the standard DNS zone-file presentation format. The miekg/dns library correctly parses this format and creates separate character-strings in the wire format per RFC 1035. DNS clients will receive the correct data.
Signed-off-by: Aurora Gaffney <aurora@blinklabs.io>
agaffney
force-pushed
the
feat/handshake-domain-storage
branch
from
e7e764f to
d0b9709
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/state/state.go (1)
223-304: Fix deletion loop: invalidstrings.SplitSeqcall and incorrectrangeusage.This block currently won’t compile and also doesn’t behave as intended:
strings.SplitSeqdoes not exist in the standardstringspackage.for tmpRecordKey := range domainRecordsSplititerates indices (ints), but the body treatstmpRecordKeyas a string key (== "", passed toslices.Contains,[]byte(tmpRecordKey)).Both are compile‑time/logic bugs. You likely meant to use
strings.Splitand iterate over the values:- domainRecordsSplit := strings.SplitSeq(string(domainRecordsVal), ",") - for tmpRecordKey := range domainRecordsSplit { - if tmpRecordKey == "" { - continue - } - if !slices.Contains(recordKeys, tmpRecordKey) { - if err := txn.Delete([]byte(tmpRecordKey)); err != nil { - return err - } - } - } + domainRecordsSplit := strings.Split(string(domainRecordsVal), ",") + for _, tmpRecordKey := range domainRecordsSplit { + if tmpRecordKey == "" { + continue + } + if !slices.Contains(recordKeys, tmpRecordKey) { + if err := txn.Delete([]byte(tmpRecordKey)); err != nil { + return err + } + } + }With this change, obsolete record keys referenced in the tracking key will be correctly deleted.
🧹 Nitpick comments (2)
internal/state/state.go (1)
306-360: Iterator lifetime insidelookupRecordsloop could be tightened.Functionality looks correct, but you create a new iterator per
recordTypeanddefer it.Close()inside the loop. With a small fixed set of types this is fine, but it’s more idiomatic (and slightly clearer) to close explicitly:- for _, recordType := range recordTypes { + for _, recordType := range recordTypes { keyPrefix := fmt.Appendf( nil, "%s%s_%s_", recordKeyPrefix, strings.ToUpper(recordType), recordName, ) - it := txn.NewIterator(badger.DefaultIteratorOptions) - defer it.Close() - for it.Seek(keyPrefix); it.ValidForPrefix(keyPrefix); it.Next() { + it := txn.NewIterator(badger.DefaultIteratorOptions) + for it.Seek(keyPrefix); it.ValidForPrefix(keyPrefix); it.Next() { item := it.Item() val, err := item.ValueCopy(nil) if err != nil { return err } // decode ... } + it.Close() }Not mandatory, but avoids accumulating defers if
recordTypesever grows.internal/indexer/handshake.go (1)
158-292: Record conversion helper looks correct; consider behavior for unknown record types.Overall mapping from Handshake records to
state.DomainRecordis solid:
- Consistent use of
dns.CanonicalName(domainName)for owner names of DS/NS/TXT and glue NS records.- Glue4/Glue6 correctly emit both NS and corresponding A/AAAA records with
Lhsset to the nameserver andRhsto the IP string.- Synth4 now safely handles
To4()with a nil check before encoding, preventing the nil‑pointer panic raised in the earlier review.- Synth4/Synth6 build
_BASE32._synth.names and emit both the delegation NS and the A/AAAA record for that synthetic name.- TXT values are joined into a single presentation‑format string with each item quoted.
One design choice to revisit is the default behavior on unsupported record types:
default: return nil, fmt.Errorf("unsupported record type: %T", record)This causes the entire domain update (and thus the block processing path) to fail if a new or unexpected Handshake resource record appears. If forward‑compatibility and robustness are priorities, you might instead log and skip such records, e.g.:
- default: - return nil, fmt.Errorf("unsupported record type: %T", record) + default: + slog.Warn("unsupported Handshake resource record; skipping", + "type", fmt.Sprintf("%T", record), + "domain", domainName, + )(Or return a typed error that the caller can choose to downgrade to a warning.)
Leaving
Ttlat the zero value for all records is consistent with the existing design where TTL is applied at query time rather than persisted per record. Based on learnings, this is intentional and acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/indexer/handshake.go(2 hunks)internal/state/state.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-02T19:35:50.114Z
Learnt from: agaffney
Repo: blinklabs-io/cdnsd PR: 455
File: internal/indexer/handshake.go:158-288
Timestamp: 2025-12-02T19:35:50.114Z
Learning: In the cdnsd codebase, DNS records stored in DomainRecord structs (internal/state/state.go) can have a TTL value of 0. The TTL is handled at query time when no TTL is specified in the stored record, so leaving the Ttl field at its zero value is intentional and acceptable.
Applied to files:
internal/indexer/handshake.go
🧬 Code graph analysis (1)
internal/indexer/handshake.go (4)
internal/state/state.go (2)
GetState(421-423)DomainRecord(43-48)internal/handshake/covenant.go (2)
UpdateCovenant(366-370)UpdateCovenant(372-372)internal/handshake/domain.go (1)
DomainResourceData(55-58)internal/handshake/transaction.go (1)
Address(185-188)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/state/state.go (2)
31-36: Prefix changes look consistent and backward‑compatible.Renaming
recordKeyPrefix/domainKeyPrefixto Cardano‑specific variants while keeping the underlying strings ("r_","d_") preserves existing data, and the newhs_*prefixes cleanly isolate Handshake records and domains.
398-419: Handshake domain wrappers correctly reuse generic helpers.
UpdateHandshakeDomainandLookupHandshakeRecordscleanly delegate toupdateDomain/lookupRecordswith thehs_*prefixes, mirroring the Cardano path and keeping concerns separated by keyspace.internal/indexer/handshake.go (2)
9-19: New imports are appropriate and used.
encoding/base32andgithub.com/miekg/dnsare both used inhandshakeResourceDataToDomainRecordsfor synth names and canonicalization; no unused imports here.
127-152: Handshake covenant handling integrates cleanly with state layer.The added calls to
handshakeResourceDataToDomainRecordsandUpdateHandshakeDomainfor bothRegisterCovenantandUpdateCovenantcorrectly:
- Resolve the human‑readable name from
NameHash.- Convert resource data once via the helper.
- Persist records under the handshake‑specific keyspace via
UpdateHandshakeDomain.- Propagate any conversion or persistence errors up the sync path.
This wiring looks sound.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Summary by cubic
Converts Handshake domain resource data into DNS records and stores them during registration and updates. This enables lookups for Handshake domains and prepares serving DNS responses.
Written for commit d0b9709. Summary will update automatically on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.