Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Soroban config_settings transformation pipeline to include additional protocol config fields (P23) and CAP-77 “frozen ledger keys” fields (P26), ensuring they are emitted in both JSON and Parquet outputs.
Changes:
- Added new P23 and P26 fields to the
ConfigSettingOutput/ConfigSettingOutputParquetschemas. - Populated the new fields in
TransformConfigSettingand mapped them throughToParquet(). - Updated config-setting transform tests to include new P23 config setting IDs.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/transform/schema.go | Adds new JSON fields to ConfigSettingOutput for P23/P26 config settings. |
| internal/transform/schema_parquet.go | Adds matching Parquet schema fields for P23/P26 config settings. |
| internal/transform/parquet_converter.go | Maps the new config-setting fields into the Parquet output struct. |
| internal/transform/config_setting.go | Extracts new P23/P26 values and introduces serializers for CAP-77 fields. |
| internal/transform/config_setting_test.go | Adds test fixtures/expectations for the new P23 config setting IDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/transform/config_setting.go
Outdated
| // serializeFrozenKeysDelta converts a FrozenLedgerKeysDelta to a JSON object with keysToFreeze and keysToUnfreeze arrays. | ||
| func serializeFrozenKeysDelta(delta xdr.FrozenLedgerKeysDelta) string { | ||
| if len(delta.KeysToFreeze) == 0 && len(delta.KeysToUnfreeze) == 0 { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
The doc comment for serializeFrozenKeysDelta mentions keysToFreeze/keysToUnfreeze, but the JSON output uses snake_case fields ("keys_to_freeze" / "keys_to_unfreeze"). Please align the comment with the actual output shape so consumers aren’t misled.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
internal/transform/config_setting.go
Outdated
| // serializeEncodedLedgerKeys converts a slice of EncodedLedgerKey (opaque bytes) to a JSON array of base64 strings. | ||
| func serializeEncodedLedgerKeys(keys []xdr.EncodedLedgerKey) string { | ||
| if len(keys) == 0 { | ||
| return "" | ||
| } | ||
| encoded := make([]string, 0, len(keys)) | ||
| for _, key := range keys { | ||
| encoded = append(encoded, base64.StdEncoding.EncodeToString(key)) | ||
| } | ||
| result, _ := json.Marshal(encoded) |
There was a problem hiding this comment.
Instead of a serialized string we might be better off as an array of strings ([]string).
And instead of encoding and marshaling like above you can use the xdr.MarshalBase64
An example of this can be found here
We can check if we are doing the marshaling and conversions correctly because these should all be real ledger keys as in we should be able to see the ledger key for the contract data or trustline or whatever like
Contract Data Ledger keys
* ABC123
* EFG456
Frozen Ledger Keys
* ABC123 -- this ledger key should be the same format as the ledger key above
internal/transform/config_setting.go
Outdated
| frozenLedgerKeysDelta, _ := configSetting.GetFrozenLedgerKeysDelta() | ||
| frozenLedgerKeysDeltaJSON := serializeFrozenKeysDelta(frozenLedgerKeysDelta) |
There was a problem hiding this comment.
I'd recommend flattening this out so that you just have like frozenLedgerKeysToFreeze and frozenLedgerKeysToUnfreeze instead of a new map to hold both entries
For example in BigQuery this would just look like
select
frozen_ledger_keys_to_freeze
, frozen_ledger_keys_to_unfreezevs
select
frozen_ledger_keys_delta.keys_to_freeze
, frozen_ledger_keys_delta.keys_to_unfreeze
internal/transform/config_setting.go
Outdated
| freezeBypassTxsDelta, _ := configSetting.GetFreezeBypassTxsDelta() | ||
| freezeBypassTxsDeltaJSON := serializeFreezeBypassTxsDelta(freezeBypassTxsDelta) |
There was a problem hiding this comment.
Same recommendation as https://github.com/stellar/stellar-etl/pull/401/changes#r3041684008
Flatten instead of making it a map
internal/transform/config_setting.go
Outdated
| func serializeHashes(hashes []xdr.Hash) string { | ||
| if len(hashes) == 0 { | ||
| return "" | ||
| } | ||
| encoded := make([]string, 0, len(hashes)) | ||
| for _, h := range hashes { | ||
| encoded = append(encoded, hex.EncodeToString(h[:])) | ||
| } | ||
| result, _ := json.Marshal(encoded) | ||
| return string(result) | ||
| } |
There was a problem hiding this comment.
Similar to https://github.com/stellar/stellar-etl/pull/401/changes#r3041658556
I think you can convert the hash to a transaction_hash like here
We would need to confirm the frozen/unfrozen tx hashes match actual tx hashing or whatever hashes == hashes we are comparing
internal/transform/schema.go
Outdated
| FrozenLedgerKeys string `json:"frozen_ledger_keys"` | ||
| FrozenLedgerKeysDelta string `json:"frozen_ledger_keys_delta"` | ||
| FreezeBypassTxs string `json:"freeze_bypass_txs"` | ||
| FreezeBypassTxsDelta string `json:"freeze_bypass_txs_delta"` |
There was a problem hiding this comment.
Update these to []string to work the the above suggested changes
PR Checklist
PR Structure
otherwise).
Thoroughness
Release planning
semver, and I've changed the name of the BRANCH to major/_ , minor/_ or patch/* .
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]