fix yaml unmarshal converted bigInt that overflow uint64 as string#747
fix yaml unmarshal converted bigInt that overflow uint64 as string#747huangzhen1997 wants to merge 11 commits intomainfrom
Conversation
|
…te-durable-pipeline
There was a problem hiding this comment.
Pull request overview
This pull request adds helper functionality to address a YAML SDK issue where big integers that overflow uint64 are incorrectly parsed as strings instead of being preserved as numeric types. The solution introduces a new helper package with functions to recursively traverse YAML-unmarshaled data structures and coerce string representations of large numbers back to *big.Int.
Changes:
- New helper package with YAML coercion utilities for handling big integers
- Integration of the helper across multiple YAML unmarshaling locations in the codebase
- Pattern matching to selectively apply coercion only to specific keys to avoid unintended conversions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| helper/yaml.go | New file containing coercion logic for converting string representations of large integers to *big.Int with pattern matching support |
| operations/operation_test.go | Added helper import and coercion call in existing test |
| experimental/analyzer/renderer_markdown.go | Applied coercion after YAML unmarshaling in markdown rendering |
| engine/cld/legacy/cli/commands/durable_pipeline_yaml.go | Applied coercion to durable pipeline YAML parsing |
| engine/cld/legacy/cli/commands/durable-pipelines.go | Applied coercion to durable pipeline file parsing |
| engine/cld/config/network/metadata.go | Applied coercion in metadata decoding |
| engine/cld/config/network/config.go | Applied coercion in network config loading |
| engine/cld/config/network/config_test.go | Applied coercion in test for network config |
| engine/cld/config/env/config_test.go | Applied coercion in test for environment config |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // elements in a list don't have their own key, so we use the parent key with a ".[]" suffix | ||
| // to check if we should coerce big ints in this list. | ||
| key := currentKey + ".[]" | ||
| x[i] = coerceBigIntStrings(x[i], key, matchFunc).(map[string]any) |
There was a problem hiding this comment.
The type assertion on line 40 can panic if coerceBigIntStrings returns a type that isn't map[string]any. While the []map[string]any case should only contain map[string]any elements, if the recursive call encounters an issue or if the YAML structure changes during coercion, this could panic. Consider using the two-value form of type assertion to handle this case more gracefully.
| x[i] = coerceBigIntStrings(x[i], key, matchFunc).(map[string]any) | |
| coerced := coerceBigIntStrings(x[i], key, matchFunc) | |
| if coercedMap, ok := coerced.(map[string]any); ok { | |
| x[i] = coercedMap | |
| } |
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | ||
| matchFunc := helper.DefaultMatchKeysToFix | ||
| yamlInput = helper.CoerceBigIntStringsForKeys(yamlInput, matchFunc) | ||
|
|
There was a problem hiding this comment.
The test Test_Operation_AsUntypedRelaxed_WithYAMLUnmarshaling includes a call to CoerceBigIntStringsForKeys with the comment "Coerce big int strings as YAML parsing may interpret large numbers as strings", but the test YAML data only contains small integers (10 and 20) that don't overflow int64 or uint64. This test doesn't actually verify the big int coercion functionality. The keys ".A" and ".B" also don't match any of the patterns in DefaultMatchKeysToFix, so the coercion would never happen here anyway. Consider either removing the coercion call from this test (since it's not needed for the test's purpose) or adding a specific test that validates the big int coercion with appropriate test data and matching keys.
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | |
| matchFunc := helper.DefaultMatchKeysToFix | |
| yamlInput = helper.CoerceBigIntStringsForKeys(yamlInput, matchFunc) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func stringToBigIntIfOverflowInt64(s string) (*big.Int, bool) { | ||
| // If it fits int64, keep it as a string (likely user meant a string, or YAML had quotes). | ||
| if _, err := strconv.ParseInt(s, 10, 64); err == nil { | ||
| return nil, false | ||
| } else { | ||
| var ne *strconv.NumError | ||
| if errors.As(err, &ne) && !errors.Is(ne.Err, strconv.ErrRange) { | ||
| // not a range overflow; should be safe to treat as string | ||
| return nil, false | ||
| } | ||
| } | ||
|
|
||
| // If it fits uint64, keep it as a string (likely user meant a string, or YAML had quotes). | ||
| if _, err := strconv.ParseUint(s, 10, 64); err == nil { | ||
| return nil, false | ||
| } else { | ||
| var ne *strconv.NumError | ||
| if errors.As(err, &ne) && !errors.Is(ne.Err, strconv.ErrRange) { | ||
| // not a range overflow; should be safe to treat as string | ||
| return nil, false | ||
| } | ||
| } | ||
|
|
||
| z, ok := new(big.Int).SetString(s, 10) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
|
|
||
| return z, true | ||
| } |
There was a problem hiding this comment.
This new YAML coercion logic isn’t covered by a dedicated unit test. Adding helper/yaml_test.go with cases for: (1) an overflow-uint64 numeric string that should become *big.Int, (2) values that fit uint64 that should remain strings, and (3) non-numeric strings that should remain unchanged would prevent regressions.
|
|
||
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | ||
| matchFunc := helper.DefaultMatchKeysToFix | ||
| dpYAML = helper.CoerceBigIntStringsForKeys(dpYAML, matchFunc).(durablePipelineYAML) |
There was a problem hiding this comment.
CoerceBigIntStringsForKeys only walks map[string]any / []any / string; dpYAML is a struct, so this call is effectively a no-op and won’t coerce values inside dpYAML.Changesets. Consider applying coercion to dpYAML.Changesets (or an untyped YAML decode) before further processing; also note this file already deals with map[interface{}]interface{}, which the coercer currently doesn’t handle.
| dpYAML = helper.CoerceBigIntStringsForKeys(dpYAML, matchFunc).(durablePipelineYAML) | |
| if dpYAML.Changesets != nil { | |
| dpYAML.Changesets = helper.CoerceBigIntStringsForKeys(dpYAML.Changesets, matchFunc) | |
| } |
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | ||
| matchFunc := helper.DefaultMatchKeysToFix | ||
| dpFile = helper.CoerceBigIntStringsForKeys(dpFile, matchFunc).(durablePipelineFile) | ||
|
|
There was a problem hiding this comment.
dpFile is a typed struct whose Changesets field is a yaml.Node; CoerceBigIntStringsForKeys currently doesn’t traverse structs or yaml.Node, so this coercion won’t have any effect. If coercion is needed here, apply it to the resolved untyped payload/map (e.g., after decode) or extend the helper to handle yaml.Node/structs via reflection.
| // converts *string* values that look like integers AND overflow int64 into *big.Int. | ||
| // It mutates map[string]any and []any in-place. | ||
| func CoerceBigIntStrings(v any) any { | ||
| return coerceBigIntStrings(v, "", nil) | ||
| } | ||
|
|
||
| // CoerceBigIntStringsForKeys is the safer variant: only converts numeric strings | ||
| // under specific leaf keys (e.g. "maxfeejuelspermsg"). |
There was a problem hiding this comment.
CoerceBigIntStrings/CoerceBigIntStringsForKeys docstring says it converts strings that overflow int64, but stringToBigIntIfOverflowInt64 only returns a *big.Int when the value overflows uint64 as well (values that fit in uint64 are left as strings). Please align the naming/docs with the actual behavior (e.g., rename helper to indicate uint64 overflow, or change the logic to match the comment).
| // converts *string* values that look like integers AND overflow int64 into *big.Int. | |
| // It mutates map[string]any and []any in-place. | |
| func CoerceBigIntStrings(v any) any { | |
| return coerceBigIntStrings(v, "", nil) | |
| } | |
| // CoerceBigIntStringsForKeys is the safer variant: only converts numeric strings | |
| // under specific leaf keys (e.g. "maxfeejuelspermsg"). | |
| // converts *string* values that look like integers AND overflow uint64 into *big.Int. | |
| // (Values that fit within int64 or uint64 are left as strings.) It mutates map[string]any | |
| // and []any in-place. | |
| func CoerceBigIntStrings(v any) any { | |
| return coerceBigIntStrings(v, "", nil) | |
| } | |
| // CoerceBigIntStringsForKeys is the safer variant: only converts numeric strings that | |
| // overflow uint64 under specific leaf keys (e.g. "maxfeejuelspermsg"). |
| func coerceBigIntStrings(v any, currentKey string, matchFunc KeyMatchFunc) any { | ||
| switch x := v.(type) { | ||
| case map[string]any: | ||
| for k, vv := range x { | ||
| key := currentKey + "." + k | ||
| x[k] = coerceBigIntStrings(vv, key, matchFunc) | ||
| } | ||
|
|
||
| return x | ||
|
|
||
| case []map[string]any: | ||
| for i := range x { | ||
| // elements in a list don't have their own key, so we use the parent key with a ".[]" suffix | ||
| // to check if we should coerce big ints in this list. | ||
| key := currentKey + ".[]" | ||
| x[i] = coerceBigIntStrings(x[i], key, matchFunc).(map[string]any) | ||
| } | ||
|
|
||
| return x | ||
|
|
||
| case []any: | ||
| for i := range x { | ||
| // elements in a list don't have their own key, so we use the parent key with a ".[]" suffix | ||
| // to check if we should coerce big ints in this list. | ||
| key := currentKey + ".[]" | ||
| x[i] = coerceBigIntStrings(x[i], key, matchFunc) | ||
| } | ||
|
|
||
| return x | ||
|
|
||
| case string: | ||
| if matchFunc != nil { | ||
| if !matchFunc(currentKey) { | ||
| return x | ||
| } | ||
| } | ||
| if bi, ok := stringToBigIntIfOverflowInt64(x); ok { | ||
| return bi // IMPORTANT: *big.Int (pointer), not big.Int (value) | ||
| } | ||
|
|
||
| return x | ||
| default: | ||
| return v | ||
| } |
There was a problem hiding this comment.
coerceBigIntStrings only handles map[string]any, []any, and string. Callers in this PR pass typed structs (e.g., Config, durablePipelineYAML, generic T), so the traversal becomes a no-op and the coercion never reaches nested any/map fields. Consider either (a) applying coercion to the untyped YAML structure before decoding into structs, or (b) extending the walker to handle structs/pointers via reflection so these call sites actually do something.
| func DefaultMatchKeysToFix(key string) bool { | ||
| patterns := []string{ | ||
| // Matching examples: | ||
| // .[].0006_deploy_evm_chain_contracts.payload.contractparamsperchain.7759470850252068959.feequoterparams.maxfeejuelspermsg | ||
| `^.*\.contractparamsperchain\.\d+\.feequoterparams\.maxfeejuelspermsg$`, | ||
| // Matching examples: | ||
| // .[].0009_deploy_ton_ccip_contracts.payload.chains.13879075125137744094.maxfeejuelspermsg | ||
| `^.*\.chains\.\d+\.maxfeejuelspermsg$`, | ||
| // Matching examples: | ||
| // .[].0016_configure_lanes.payload.lanes.[].chaina.tokenprices.EQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd99 | ||
| // .[].0016_configure_lanes.payload.lanes.[].chainb.tokenprices.0xb1D4538B4571d411F07960EF2838Ce337FE1E80E | ||
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[a-zA-Z0-9]+$`, | ||
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.feequoterdestchainconfig\.gasmultiplierweipereth$`, | ||
| } | ||
|
|
||
| matched := false | ||
| for _, p := range patterns { | ||
| var err error | ||
| matched, err = regexp.MatchString(p, key) | ||
| if err != nil { | ||
| break | ||
| } | ||
| if matched { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return matched |
There was a problem hiding this comment.
DefaultMatchKeysToFix recompiles regexes on every call (patterns slice allocation + regexp.MatchString compilation). Since this match function may be invoked for many scalar nodes during a walk, consider precompiling the regexps once at package init (regexp.MustCompile) and then iterating over []*regexp.Regexp for better performance and simpler error handling.
| func DefaultMatchKeysToFix(key string) bool { | |
| patterns := []string{ | |
| // Matching examples: | |
| // .[].0006_deploy_evm_chain_contracts.payload.contractparamsperchain.7759470850252068959.feequoterparams.maxfeejuelspermsg | |
| `^.*\.contractparamsperchain\.\d+\.feequoterparams\.maxfeejuelspermsg$`, | |
| // Matching examples: | |
| // .[].0009_deploy_ton_ccip_contracts.payload.chains.13879075125137744094.maxfeejuelspermsg | |
| `^.*\.chains\.\d+\.maxfeejuelspermsg$`, | |
| // Matching examples: | |
| // .[].0016_configure_lanes.payload.lanes.[].chaina.tokenprices.EQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd99 | |
| // .[].0016_configure_lanes.payload.lanes.[].chainb.tokenprices.0xb1D4538B4571d411F07960EF2838Ce337FE1E80E | |
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[a-zA-Z0-9]+$`, | |
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.feequoterdestchainconfig\.gasmultiplierweipereth$`, | |
| } | |
| matched := false | |
| for _, p := range patterns { | |
| var err error | |
| matched, err = regexp.MatchString(p, key) | |
| if err != nil { | |
| break | |
| } | |
| if matched { | |
| break | |
| } | |
| } | |
| return matched | |
| var defaultMatchKeyRegexps = []*regexp.Regexp{ | |
| // Matching examples: | |
| // .[].0006_deploy_evm_chain_contracts.payload.contractparamsperchain.7759470850252068959.feequoterparams.maxfeejuelspermsg | |
| regexp.MustCompile(`^.*\.contractparamsperchain\.\d+\.feequoterparams\.maxfeejuelspermsg$`), | |
| // Matching examples: | |
| // .[].0009_deploy_ton_ccip_contracts.payload.chains.13879075125137744094.maxfeejuelspermsg | |
| regexp.MustCompile(`^.*\.chains\.\d+\.maxfeejuelspermsg$`), | |
| // Matching examples: | |
| // .[].0016_configure_lanes.payload.lanes.[].chaina.tokenprices.EQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd99 | |
| // .[].0016_configure_lanes.payload.lanes.[].chainb.tokenprices.0xb1D4538B4571d411F07960EF2838Ce337FE1E80E | |
| regexp.MustCompile(`^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[a-zA-Z0-9]+$`), | |
| regexp.MustCompile(`^.*\.lanes\.\[\]\.(?:chaina|chainb)\.feequoterdestchainconfig\.gasmultiplierweipereth$`), | |
| } | |
| func DefaultMatchKeysToFix(key string) bool { | |
| for _, re := range defaultMatchKeyRegexps { | |
| if re.MatchString(key) { | |
| return true | |
| } | |
| } | |
| return false |
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | ||
| matchFunc := helper.DefaultMatchKeysToFix | ||
| yamlInput = helper.CoerceBigIntStringsForKeys(yamlInput, matchFunc) | ||
|
|
There was a problem hiding this comment.
In this test YAML, the keys are A/B so DefaultMatchKeysToFix won’t match and the coercion call will be a no-op. If the intent is to exercise the overflow-uint64 YAML-string fix, adjust the test YAML to include a key path that matches the matchFunc and a value > uint64, and assert it becomes *big.Int (or call CoerceBigIntStrings without key filtering for this test).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | ||
| matchFunc := helper.DefaultMatchKeysToFix | ||
| dpFile = helper.CoerceBigIntStringsForKeys(dpFile, matchFunc).(durablePipelineFile) |
There was a problem hiding this comment.
This type assertion can panic if CoerceBigIntStringsForKeys doesn't return a durablePipelineFile. The helper function recursively processes the data structure and returns interface{}, so there's no compile-time guarantee that the returned value will be of the expected type. If the YAML structure changes or contains unexpected types, this will panic at runtime. Consider using the two-value form: if result, ok := helper.CoerceBigIntStringsForKeys(dpFile, matchFunc).(durablePipelineFile); ok { dpFile = result } else { return fmt.Errorf("failed to coerce big int strings") }.
| dpFile = helper.CoerceBigIntStringsForKeys(dpFile, matchFunc).(durablePipelineFile) | |
| coerced := helper.CoerceBigIntStringsForKeys(dpFile, matchFunc) | |
| result, ok := coerced.(durablePipelineFile) | |
| if !ok { | |
| return fmt.Errorf("failed to coerce big int strings to durablePipelineFile, got %T", coerced) | |
| } | |
| dpFile = result |
| // Matching examples: | ||
| // .[].0016_configure_lanes.payload.lanes.[].chaina.tokenprices.EQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd99 | ||
| // .[].0016_configure_lanes.payload.lanes.[].chainb.tokenprices.0xb1D4538B4571d411F07960EF2838Ce337FE1E80E | ||
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[a-zA-Z0-9]+$`, |
There was a problem hiding this comment.
The regex pattern uses [a-zA-Z0-9]+ which only matches alphanumeric characters, but the test case shows token addresses like "0xb1D4538B4571d411F07960EF2838Ce337FE1E80E" which contain lowercase 'x' after '0x'. While this particular address will match, TON addresses like "EQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd99" from the comment on line 113 will also match. However, if token addresses can contain other characters (like underscores, hyphens, or other special characters), this pattern may be too restrictive. Consider whether the pattern should be more permissive, e.g., [a-zA-Z0-9_-]+ or [^\\.]+.
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[a-zA-Z0-9]+$`, | |
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[^.]+$`, |
|
|
||
| // CoerceBigIntStrings walks a yaml.Unmarshal result (maps/slices/scalars) and | ||
| // converts *string* values that look like integers AND overflow int64 into *big.Int. | ||
| // It mutates map[string]any and []any in-place. |
There was a problem hiding this comment.
The function documentation states "It mutates map[string]any and []any in-place" (line 14), but callers are reassigning the result back to the original variable (e.g., line 186 in operations/operation_test.go: yamlInput = helper.CoerceBigIntStringsForKeys(yamlInput, matchFunc)). This is correct since for scalars (strings, ints, etc.) the function returns a new value rather than mutating. However, the documentation could be clearer about when mutation occurs vs when a new value is returned. Consider clarifying: "It mutates map[string]any and []any in-place, but returns new values for scalars."
| // It mutates map[string]any and []any in-place. | |
| // It mutates map[string]any and []any in-place, but returns new values for scalar values. |
| func TestCoerceBigIntStrings(t *testing.T) { | ||
| bigVal := "18446744073709551616" // overflows uint64 | ||
| expectedBig, _ := new(big.Int).SetString(bigVal, 10) | ||
|
|
||
| t.Run("nil and scalars pass through", func(t *testing.T) { | ||
| assert.Nil(t, CoerceBigIntStrings(nil)) | ||
| assert.Equal(t, 42, CoerceBigIntStrings(42)) | ||
| assert.Equal(t, "12345", CoerceBigIntStrings("12345")) | ||
| }) | ||
|
|
||
| t.Run("overflow string converted to big.Int", func(t *testing.T) { | ||
| result := CoerceBigIntStrings(bigVal) | ||
| bi, ok := result.(*big.Int) | ||
| require.True(t, ok, "expected *big.Int, got %T", result) | ||
| assert.Equal(t, expectedBig, bi) | ||
| }) | ||
|
|
||
| t.Run("map with mixed values", func(t *testing.T) { | ||
| input := map[string]any{ | ||
| "name": "test", | ||
| "value": bigVal, | ||
| "count": 5, | ||
| } | ||
| result := CoerceBigIntStrings(input).(map[string]any) | ||
| assert.Equal(t, "test", result["name"]) | ||
| assert.Equal(t, expectedBig, result["value"]) | ||
| assert.Equal(t, 5, result["count"]) | ||
| }) | ||
|
|
||
| t.Run("nested maps and slices", func(t *testing.T) { | ||
| input := map[string]any{ | ||
| "level1": map[string]any{ | ||
| "level2": []any{ | ||
| map[string]any{"level3": bigVal}, | ||
| }, | ||
| }, | ||
| } | ||
| result := CoerceBigIntStrings(input).(map[string]any) | ||
| l1 := result["level1"].(map[string]any) | ||
| l2 := l1["level2"].([]any) | ||
| l3 := l2[0].(map[string]any) | ||
| assert.Equal(t, expectedBig, l3["level3"]) | ||
| }) | ||
|
|
||
| t.Run("slice of typed maps", func(t *testing.T) { | ||
| input := []map[string]any{ | ||
| {"key": bigVal}, | ||
| {"key": "small"}, | ||
| } | ||
| result := CoerceBigIntStrings(input).([]map[string]any) | ||
| assert.Equal(t, expectedBig, result[0]["key"]) | ||
| assert.Equal(t, "small", result[1]["key"]) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The test doesn't demonstrate the actual YAML parsing issue this PR aims to fix. The test uses hardcoded values like "18446744073709551616" as strings, but doesn't show a case where YAML unmarshaling converts a large number to a string (which is the problem described in the PR). Consider adding a test that unmarshals actual YAML containing large numbers without quotes and verifies they are correctly converted to big.Int after applying the helper function.
| result := CoerceBigIntStringsForKeys(input, func(string) bool { return true }).(map[string]any) | ||
| assert.Equal(t, expectedBig, result["target"]) | ||
|
|
||
| // matchNone keeps strings | ||
| input = map[string]any{"target": bigVal} | ||
| result = CoerceBigIntStringsForKeys(input, func(string) bool { return false }).(map[string]any) | ||
| assert.Equal(t, bigVal, result["target"]) | ||
| }) | ||
|
|
||
| t.Run("selective key matching", func(t *testing.T) { | ||
| matchTarget := func(key string) bool { return key == ".target" } | ||
| input := map[string]any{"target": bigVal, "other": bigVal} | ||
| result := CoerceBigIntStringsForKeys(input, matchTarget).(map[string]any) | ||
| assert.Equal(t, expectedBig, result["target"]) | ||
| assert.Equal(t, bigVal, result["other"]) | ||
| }) | ||
|
|
||
| t.Run("nested and list key paths", func(t *testing.T) { | ||
| matchNested := func(key string) bool { return key == ".outer.inner" } | ||
| input := map[string]any{ | ||
| "outer": map[string]any{"inner": bigVal, "skip": bigVal}, | ||
| } | ||
| result := CoerceBigIntStringsForKeys(input, matchNested).(map[string]any) | ||
| inner := result["outer"].(map[string]any) | ||
| assert.Equal(t, expectedBig, inner["inner"]) | ||
| assert.Equal(t, bigVal, inner["skip"]) | ||
|
|
||
| matchList := func(key string) bool { return key == ".items.[].val" } | ||
| input2 := map[string]any{ | ||
| "items": []map[string]any{{"val": bigVal}}, | ||
| } | ||
| result2 := CoerceBigIntStringsForKeys(input2, matchList).(map[string]any) | ||
| items := result2["items"].([]map[string]any) | ||
| assert.Equal(t, expectedBig, items[0]["val"]) |
There was a problem hiding this comment.
The test uses unsafe type assertions (lines 100, 105, 112, 122, 123, 131, 132) that will panic if the returned type is incorrect. While these are in test code and panic is acceptable for test failures, it would be more descriptive to use require.IsType or the two-value form of type assertion with an explicit assertion, so test output clearly indicates what went wrong.
| } | ||
|
|
||
| func stringToBigIntIfOverflowInt64(s string) (*big.Int, bool) { | ||
| // If it fits int64, keep it as a string (likely user meant a string, or YAML had quotes). |
There was a problem hiding this comment.
The comment mentions "int64" but should refer to "uint64" since the function checks for values that overflow uint64, not int64. The function only converts strings to big.Int if they overflow both int64 AND uint64.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CoerceBigIntStrings walks a yaml.Unmarshal result (maps/slices/scalars) and | ||
| // converts *string* values that look like integers AND overflow int64 into *big.Int. | ||
| // It mutates map[string]any and []any in-place. |
There was a problem hiding this comment.
The docstring says this converts strings that overflow int64, but stringToBigIntIfOverflowInt64 only converts when the value overflows uint64 (values that fit uint64 but not int64 are intentionally left as strings). Please update the comments (and/or rename the helper) so behavior and naming match what the function actually does.
| return x | ||
|
|
There was a problem hiding this comment.
coerceBigIntStrings does not handle map[interface{}]interface{} (a common result from YAML decoding in parts of this repo, e.g. the JSON-safe conversion helpers). That means large numeric strings nested under those maps will not be coerced. Consider adding support for map[any]any/map[interface{}]interface{} (and converting keys to string for path matching) so coercion works consistently.
| return x | |
| return x | |
| case map[interface{}]interface{}: | |
| for k, vv := range x { | |
| ks, ok := k.(string) | |
| if !ok { | |
| // Skip non-string keys since path matching is string-based. | |
| continue | |
| } | |
| key := currentKey + "." + ks | |
| x[k] = coerceBigIntStrings(vv, key, matchFunc) | |
| } | |
| return x |
| func DefaultMatchKeysToFix(key string) bool { | ||
| patterns := []string{ | ||
| // Matching examples: | ||
| // .[].0006_deploy_evm_chain_contracts.payload.contractparamsperchain.7759470850252068959.feequoterparams.maxfeejuelspermsg | ||
| `^.*\.contractparamsperchain\.\d+\.feequoterparams\.maxfeejuelspermsg$`, | ||
| // Matching examples: | ||
| // .[].0009_deploy_ton_ccip_contracts.payload.chains.13879075125137744094.maxfeejuelspermsg | ||
| `^.*\.chains\.\d+\.maxfeejuelspermsg$`, | ||
| // Matching examples: | ||
| // .[].0016_configure_lanes.payload.lanes.[].chaina.tokenprices.EQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAd99 | ||
| // .[].0016_configure_lanes.payload.lanes.[].chainb.tokenprices.0xb1D4538B4571d411F07960EF2838Ce337FE1E80E | ||
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.tokenprices\.[a-zA-Z0-9]+$`, | ||
| `^.*\.lanes\.\[\]\.(?:chaina|chainb)\.feequoterdestchainconfig\.gasmultiplierweipereth$`, | ||
| } | ||
|
|
||
| matched := false | ||
| for _, p := range patterns { | ||
| var err error | ||
| matched, err = regexp.MatchString(p, key) | ||
| if err != nil { | ||
| break | ||
| } | ||
| if matched { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
DefaultMatchKeysToFix recompiles regex patterns on every call via regexp.MatchString and re-allocates the patterns slice. Since this function may be called for every leaf during a traversal, this can become a hot path. Precompile the regexps once at package init (e.g., var defaultKeyPatterns = []*regexp.Regexp{...}) and just loop over re.MatchString(key).
| func TestStringToBigIntIfOverflowInt64(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| wantOK bool | ||
| want string | ||
| }{ | ||
| {"fits int64", "12345", false, ""}, | ||
| {"fits uint64 but not int64", "18446744073709551615", false, ""}, | ||
| {"overflows uint64", "18446744073709551616", true, "18446744073709551616"}, | ||
| {"very large number", "115792089237316195423570985008687907853269984665640564039457584007913129639935", true, "115792089237316195423570985008687907853269984665640564039457584007913129639935"}, | ||
| {"non-numeric string", "hello", false, ""}, |
There was a problem hiding this comment.
Test name TestStringToBigIntIfOverflowInt64 (and the helper name) suggests int64 overflow, but the behavior under test only converts values that overflow uint64. Renaming the test (and potentially the helper) would make intent clearer and prevent confusion about which values get coerced.
|
|
Hey Joe, you might be interested in https://github.com/smartcontractkit/chainlink-deployments/pull/10610 as a workaround |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CoerceBigIntStrings walks a yaml.Unmarshal result (maps/slices/scalars) and | ||
| // converts *string* values that look like integers AND overflow int64 into *big.Int. | ||
| // It mutates map[string]any and []any in-place. | ||
| func CoerceBigIntStrings(v any) any { | ||
| return coerceBigIntStrings(v, "", nil) |
There was a problem hiding this comment.
Coercion helper docs/naming are inconsistent with the actual behavior: CoerceBigIntStrings/stringToBigIntIfOverflowInt64 only convert when the numeric string overflows uint64 (values that fit uint64 but not int64 are intentionally left as strings per tests). Please rename/update the function and comment to reflect the real criterion (overflow uint64), or adjust the logic to match the stated int64-overflow behavior.
| // Coerce big int strings as YAML parsing may interpret large numbers as strings | ||
| matchFunc := helper.DefaultMatchKeysToFix | ||
| target = helper.CoerceBigIntStringsForKeys(target, matchFunc).(T) | ||
|
|
There was a problem hiding this comment.
CoerceBigIntStringsForKeys does not traverse structs, so calling it on the decoded target (type parameter T) will typically return the value unchanged. Also, DefaultMatchKeysToFix is tailored to durable-pipeline key paths and is unlikely to match metadata keys. If metadata needs this fix, run coercion on the untyped map/slice form (before unmarshalling into T) and/or allow callers to provide an appropriate match function.
|
close as we have merged this |
Adding helper support that address the YAML sdk issue, which converts big.int that overflow uint64 to string.