chore(payments): Add version structure with minor#103
Merged
Conversation
Contributor
WalkthroughThe payments module's version compatibility checks were refactored to compare only the major version component rather than full semantic versions. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Author
There was a problem hiding this comment.
My IDE was not happy with the state of the go mod, it would update automatically this file
Comment on lines
-44
to
-49
| res := semver.Compare(version, "v3.0.0-rc.1") | ||
| switch res { | ||
| case 0, 1: | ||
| controller.SetVersion(V3) | ||
| controller.SetVersion(*paymentVersion) | ||
| return nil | ||
| } | ||
|
|
||
| func computePaymentVersion(rawVersion string) (*Version, error) { | ||
| semverVersion := semver.MajorMinor(rawVersion) | ||
| if semverVersion == "" { | ||
| // we assume the version is a commit id | ||
| // thus corresponds to the latest possible version | ||
| return &Version{Major: V3, Minor: math.MaxInt, Raw: rawVersion}, nil | ||
| } | ||
|
|
||
| parts := strings.Split(semver.Canonical(semverVersion), ".") | ||
| if len(parts) < 2 { | ||
| return nil, fmt.Errorf("expected both major and minor for version string: %s", rawVersion) | ||
| } | ||
|
|
||
| var major PaymentMajorVersion | ||
| minor, _ := strconv.Atoi(parts[1]) | ||
|
|
||
| switch parts[0] { | ||
| case "v0", "v1", "v2": | ||
| major = V1 | ||
| case "v3": | ||
| major = V3 | ||
| default: | ||
| controller.SetVersion(V1) |
Contributor
Author
There was a problem hiding this comment.
Following that existing piece of code, V0 and V2 does not exist.
There is a lot of places in the codebase where we check for V0. Maybe we should remove those?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/payments/connectors/list.go (1)
84-125:⚠️ Potential issue | 🟠 MajorAdd a
defaultcase to avoid silent success on unsupported major versions.If
c.PaymentsVersion.Majoris not in{0,1,2,3}, this switch does nothing and the command returns success with an empty list. Please fail fast (or explicitly route) for unknown majors.Suggested fix
switch c.PaymentsVersion.Major { case versions.V3: response, err := stackClient.Payments.V3.ListConnectors(cmd.Context(), operations.V3ListConnectorsRequest{ PageSize: &pageSizeAsInt, }) @@ connectorsSlice := response.ConnectorsResponse.Data[:endIndex] c.store.Connectors = fctl.Map(connectorsSlice, V1toConnectorData) +default: + return nil, fmt.Errorf("unsupported payments major version: %d", c.PaymentsVersion.Major) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/payments/connectors/list.go` around lines 84 - 125, The switch on c.PaymentsVersion.Major in cmd/payments/connectors/list.go currently handles versions.V0–V3 but has no default branch, so unknown major versions silently do nothing; add a default case to the switch (alongside the cases for versions.V0, V1, V2, V3) that returns a clear error (e.g., fmt.Errorf("unsupported payments major version: %d", c.PaymentsVersion.Major)) so callers of the function (the code invoking the switch / the command handler) fail fast when an unrecognized major is encountered.cmd/payments/connectors/uninstall.go (1)
95-169:⚠️ Potential issue | 🟠 MajorReject unsupported majors instead of falling through as success.
Any major outside
{0,1,3}now skips all switch cases, but Lines 167-169 still mark the operation successful and the render path prints a success message with empty data. Please add an explicit fallback here, or make>= versions.V3intentional inRunas well if newer majors are supposed to reuse the V3 API.Suggested fix
switch c.PaymentsVersion.Major { case versions.V3: if connectorID == "" { return nil, fmt.Errorf("missing connector ID") } @@ c.store.Connector = provider +default: + return nil, fmt.Errorf("unsupported payments major version: %d", c.PaymentsVersion.Major) } c.store.Success = trueAlso applies to: 174-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/payments/connectors/uninstall.go` around lines 95 - 169, The switch on c.PaymentsVersion.Major currently falls through (no case hit) for unsupported majors but the function still sets c.store.Success = true; modify the switch in the Run flow to explicitly handle unsupported majors by adding a default branch that returns an error (e.g. fmt.Errorf("unsupported payments major: %d", c.PaymentsVersion.Major)) so the function does not mark success for unknown versions; alternatively, if newer majors should reuse the V3 path, change the V3 case match to a condition like ">= versions.V3" instead of only versions.V3—apply the same explicit fallback/adjustment to the similar switch later that also sets c.store.Success.
🧹 Nitpick comments (1)
cmd/payments/versions/versions.go (1)
78-79: Silently ignoringstrconv.Atoierror may mask malformed version strings.If
parts[1]is not a valid integer (e.g.,"v3.abc"),Atoireturns 0 and an error that is discarded. This could lead to unexpected behavior where an invalid minor version silently becomes0.Consider validating the conversion:
♻️ Proposed fix to handle parse error
var major PaymentMajorVersion -minor, _ := strconv.Atoi(parts[1]) +minor, err := strconv.Atoi(parts[1]) +if err != nil { + return nil, fmt.Errorf("invalid minor version in: %s", rawVersion) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/payments/versions/versions.go` around lines 78 - 79, The code currently ignores the error returned by strconv.Atoi when converting parts[1] into minor, which can silently treat invalid minor versions as 0; update the parsing in the versions handling code (referencing PaymentMajorVersion, the minor variable, and the strconv.Atoi call on parts[1]) to check the returned error, and if it is non-nil return/propagate a clear error (or handle it according to the function's error pattern) indicating a malformed version string so invalid inputs like "v3.abc" are rejected instead of becoming 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/payments/pools/delete.go`:
- Around line 73-75: The current version gate only checks
c.PaymentsVersion.Major against versions.V1; change it to require at least v3.1
by validating both c.PaymentsVersion.Major and c.PaymentsVersion.Minor (e.g., if
c.PaymentsVersion.Major < 3 || (c.PaymentsVersion.Major == 3 &&
c.PaymentsVersion.Minor < 1) { return nil, fmt.Errorf("pools are only supported
in >= v3.1.0") }) so the check uses c.PaymentsVersion.Major and
c.PaymentsVersion.Minor and returns the updated error message.
In `@cmd/payments/transferinitiation/update_status.go`:
- Around line 75-77: The version gate in update_status.go is inconsistent: it
currently checks c.PaymentsVersion.Major < versions.V1 but the error text says
">= v2.0.0"; update the check to c.PaymentsVersion.Major < versions.V2 (or if
the feature actually supports v1+, change the error string to say ">= v1.0.0")
so the conditional and the error message match—adjust the comparison symbol
(versions.V1 → versions.V2) and/or the fmt.Errorf message accordingly in the
same block where c.PaymentsVersion.Major is inspected.
---
Outside diff comments:
In `@cmd/payments/connectors/list.go`:
- Around line 84-125: The switch on c.PaymentsVersion.Major in
cmd/payments/connectors/list.go currently handles versions.V0–V3 but has no
default branch, so unknown major versions silently do nothing; add a default
case to the switch (alongside the cases for versions.V0, V1, V2, V3) that
returns a clear error (e.g., fmt.Errorf("unsupported payments major version:
%d", c.PaymentsVersion.Major)) so callers of the function (the code invoking the
switch / the command handler) fail fast when an unrecognized major is
encountered.
In `@cmd/payments/connectors/uninstall.go`:
- Around line 95-169: The switch on c.PaymentsVersion.Major currently falls
through (no case hit) for unsupported majors but the function still sets
c.store.Success = true; modify the switch in the Run flow to explicitly handle
unsupported majors by adding a default branch that returns an error (e.g.
fmt.Errorf("unsupported payments major: %d", c.PaymentsVersion.Major)) so the
function does not mark success for unknown versions; alternatively, if newer
majors should reuse the V3 path, change the V3 case match to a condition like
">= versions.V3" instead of only versions.V3—apply the same explicit
fallback/adjustment to the similar switch later that also sets c.store.Success.
---
Nitpick comments:
In `@cmd/payments/versions/versions.go`:
- Around line 78-79: The code currently ignores the error returned by
strconv.Atoi when converting parts[1] into minor, which can silently treat
invalid minor versions as 0; update the parsing in the versions handling code
(referencing PaymentMajorVersion, the minor variable, and the strconv.Atoi call
on parts[1]) to check the returned error, and if it is non-nil return/propagate
a clear error (or handle it according to the function's error pattern)
indicating a malformed version string so invalid inputs like "v3.abc" are
rejected instead of becoming 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67191a94-5884-4aa3-b679-c9bfd2981afe
📒 Files selected for processing (38)
cmd/payments/accounts/create.gocmd/payments/bankaccounts/create.gocmd/payments/bankaccounts/forward.gocmd/payments/bankaccounts/list.gocmd/payments/bankaccounts/show.gocmd/payments/bankaccounts/update_metadata.gocmd/payments/connectors/configs/adyen.gocmd/payments/connectors/configs/atlar.gocmd/payments/connectors/configs/bankingcircle.gocmd/payments/connectors/configs/currencycloud.gocmd/payments/connectors/configs/getconfig.gocmd/payments/connectors/configs/mangopay.gocmd/payments/connectors/configs/modulr.gocmd/payments/connectors/configs/moneycorp.gocmd/payments/connectors/configs/stripe.gocmd/payments/connectors/configs/wise.gocmd/payments/connectors/install/column.gocmd/payments/connectors/install/qonto.gocmd/payments/connectors/list.gocmd/payments/connectors/uninstall.gocmd/payments/payments/create.gocmd/payments/pools/add_accounts.gocmd/payments/pools/create.gocmd/payments/pools/delete.gocmd/payments/pools/list.gocmd/payments/pools/remove_account.gocmd/payments/pools/show.gocmd/payments/tasks/show.gocmd/payments/transferinitiation/approve.gocmd/payments/transferinitiation/create.gocmd/payments/transferinitiation/delete.gocmd/payments/transferinitiation/list.gocmd/payments/transferinitiation/reject.gocmd/payments/transferinitiation/retry.gocmd/payments/transferinitiation/reverse.gocmd/payments/transferinitiation/show.gocmd/payments/transferinitiation/update_status.gocmd/payments/versions/versions.go
Comment on lines
+73
to
75
| if c.PaymentsVersion.Major < versions.V1 { | ||
| return nil, fmt.Errorf("pools are only supported in >= v1.0.0") | ||
| } |
Contributor
There was a problem hiding this comment.
Pool version gate is too permissive for the stated feature baseline.
This check allows versions where pools are not expected to be available per the PR objective (v3.1+). Please gate on both major and minor here.
Proposed fix
- if c.PaymentsVersion.Major < versions.V1 {
- return nil, fmt.Errorf("pools are only supported in >= v1.0.0")
- }
+ if c.PaymentsVersion.Major < versions.V3 || (c.PaymentsVersion.Major == versions.V3 && c.PaymentsVersion.Minor < 1) {
+ return nil, fmt.Errorf("pools are only supported in >= v3.1.0")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if c.PaymentsVersion.Major < versions.V1 { | |
| return nil, fmt.Errorf("pools are only supported in >= v1.0.0") | |
| } | |
| if c.PaymentsVersion.Major < versions.V3 || (c.PaymentsVersion.Major == versions.V3 && c.PaymentsVersion.Minor < 1) { | |
| return nil, fmt.Errorf("pools are only supported in >= v3.1.0") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/payments/pools/delete.go` around lines 73 - 75, The current version gate
only checks c.PaymentsVersion.Major against versions.V1; change it to require at
least v3.1 by validating both c.PaymentsVersion.Major and
c.PaymentsVersion.Minor (e.g., if c.PaymentsVersion.Major < 3 ||
(c.PaymentsVersion.Major == 3 && c.PaymentsVersion.Minor < 1) { return nil,
fmt.Errorf("pools are only supported in >= v3.1.0") }) so the check uses
c.PaymentsVersion.Major and c.PaymentsVersion.Minor and returns the updated
error message.
Comment on lines
+75
to
77
| if c.PaymentsVersion.Major < versions.V1 { | ||
| return nil, fmt.Errorf("transfer initiation updates are only supported in >= v2.0.0") | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent error message: check is < V1 but message says >= v2.0.0.
The version check allows V1 and above (which includes actual versions v1.x, v2.x, v3.x), but the error message states the feature requires >= v2.0.0. This discrepancy could confuse users.
If the feature truly requires v2+, the check should be < versions.V2. If v1+ is correct, update the message.
🔧 Proposed fix (if v1+ is correct)
if c.PaymentsVersion.Major < versions.V1 {
- return nil, fmt.Errorf("transfer initiation updates are only supported in >= v2.0.0")
+ return nil, fmt.Errorf("transfer initiation updates are only supported in >= v1.0.0")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if c.PaymentsVersion.Major < versions.V1 { | |
| return nil, fmt.Errorf("transfer initiation updates are only supported in >= v2.0.0") | |
| } | |
| if c.PaymentsVersion.Major < versions.V1 { | |
| return nil, fmt.Errorf("transfer initiation updates are only supported in >= v1.0.0") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/payments/transferinitiation/update_status.go` around lines 75 - 77, The
version gate in update_status.go is inconsistent: it currently checks
c.PaymentsVersion.Major < versions.V1 but the error text says ">= v2.0.0";
update the check to c.PaymentsVersion.Major < versions.V2 (or if the feature
actually supports v1+, change the error string to say ">= v1.0.0") so the
conditional and the error message match—adjust the comparison symbol
(versions.V1 → versions.V2) and/or the fmt.Errorf message accordingly in the
same block where c.PaymentsVersion.Major is inspected.
laouji
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
In payments, we now have some features or connectors that are only available from a specific minor:
The current framework does not allow easily to check for the minor versioning