feat(payments): v3 implementation #107
Conversation
def8774 to
458bfe0
Compare
WalkthroughThis pull request involves a comprehensive removal of multiple files and components across the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…xternal accounts state
Switch from "ubuntu-latest" to "formance-runner" for consistency. Remove redundant setup steps in GoReleaser and env actions. Add Earthly setup in releases workflow for caching efficiency.
…ses when grpc client closes
…ve to wait as long when polling for events
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
da1f2ed to
cfa53cd
Compare
test: (e2e) implement pools e2e tests and stop using pools idempotency key as workflowID
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Earthfile (2)
82-84: Avoid unnecessary Docker image pull when not running integration testsIn the
ELSEbranch of the conditional, pulling thepostgres:15-alpineimage is unnecessary since integration tests are not being run. This can save build time and resources.Apply this diff to remove the unnecessary image pull:
ELSE - WITH DOCKER --pull=postgres:15-alpine DO --pass-args +GO_TESTS - END ENDThis removes the Docker pull command, optimizing the test execution when integration tests are excluded.
129-129: Evaluate the necessity of installingopenjdk11Installing
openjdk11increases the image size and may not be required for the generate step unless Java-based tools are used.Verify if
openjdk11is essential for the code generation process. If not, consider removing it to streamline the build:RUN apk update && apk add openjdk11If
openjdk11is not needed, you can remove this line to reduce build time and image size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/actions/env/action.ymlis excluded by!**/*.yml.github/workflows/main.ymlis excluded by!**/*.yml.github/workflows/releases.ymlis excluded by!**/*.ymlcmd/connectors/internal/connectors/atlar/Insomnium.jsonis excluded by!**/*.json
📒 Files selected for processing (77)
.gitignore(1 hunks)CODEOWNERS(1 hunks)Earthfile(3 hunks)cmd/api/internal/api/accounts.go(0 hunks)cmd/api/internal/api/accounts_test.go(0 hunks)cmd/api/internal/api/api_utils_test.go(0 hunks)cmd/api/internal/api/backend/backend.go(0 hunks)cmd/api/internal/api/backend/backend_generated.go(0 hunks)cmd/api/internal/api/balances.go(0 hunks)cmd/api/internal/api/balances_test.go(0 hunks)cmd/api/internal/api/bank_accounts.go(0 hunks)cmd/api/internal/api/bank_accounts_test.go(0 hunks)cmd/api/internal/api/health.go(0 hunks)cmd/api/internal/api/metadata.go(0 hunks)cmd/api/internal/api/metadata_test.go(0 hunks)cmd/api/internal/api/module.go(0 hunks)cmd/api/internal/api/payments.go(0 hunks)cmd/api/internal/api/payments_test.go(0 hunks)cmd/api/internal/api/pools.go(0 hunks)cmd/api/internal/api/pools_test.go(0 hunks)cmd/api/internal/api/recovery.go(0 hunks)cmd/api/internal/api/router.go(0 hunks)cmd/api/internal/api/service/accounts.go(0 hunks)cmd/api/internal/api/service/accounts_test.go(0 hunks)cmd/api/internal/api/service/balance.go(0 hunks)cmd/api/internal/api/service/bank_accounts.go(0 hunks)cmd/api/internal/api/service/errors.go(0 hunks)cmd/api/internal/api/service/payments.go(0 hunks)cmd/api/internal/api/service/payments_test.go(0 hunks)cmd/api/internal/api/service/ping.go(0 hunks)cmd/api/internal/api/service/pools.go(0 hunks)cmd/api/internal/api/service/pools_test.go(0 hunks)cmd/api/internal/api/service/service.go(0 hunks)cmd/api/internal/api/service/service_test.go(0 hunks)cmd/api/internal/api/service/transfer_initiations.go(0 hunks)cmd/api/internal/api/transfer_initiation.go(0 hunks)cmd/api/internal/api/transfer_initiation_test.go(0 hunks)cmd/api/internal/api/utils.go(0 hunks)cmd/api/internal/storage/accounts.go(0 hunks)cmd/api/internal/storage/accounts_test.go(0 hunks)cmd/api/internal/storage/balances.go(0 hunks)cmd/api/internal/storage/balances_test.go(0 hunks)cmd/api/internal/storage/bank_accounts.go(0 hunks)cmd/api/internal/storage/bank_accounts_test.go(0 hunks)cmd/api/internal/storage/connectors.go(0 hunks)cmd/api/internal/storage/connectors_test.go(0 hunks)cmd/api/internal/storage/main_test.go(0 hunks)cmd/api/internal/storage/metadata.go(0 hunks)cmd/api/internal/storage/module.go(0 hunks)cmd/api/internal/storage/paginate.go(0 hunks)cmd/api/internal/storage/payments.go(0 hunks)cmd/api/internal/storage/payments_test.go(0 hunks)cmd/api/internal/storage/ping.go(0 hunks)cmd/api/internal/storage/pools.go(0 hunks)cmd/api/internal/storage/pools_test.go(0 hunks)cmd/api/internal/storage/repository.go(0 hunks)cmd/api/internal/storage/sort.go(0 hunks)cmd/api/internal/storage/transfer_initiation.go(0 hunks)cmd/api/internal/storage/transfer_initiation_test.go(0 hunks)cmd/api/internal/storage/utils.go(0 hunks)cmd/api/root.go(0 hunks)cmd/api/serve.go(0 hunks)cmd/connectors/internal/api/api_utils_test.go(0 hunks)cmd/connectors/internal/api/backend/backend.go(0 hunks)cmd/connectors/internal/api/backend/backend_generated.go(0 hunks)cmd/connectors/internal/api/bank_account.go(0 hunks)cmd/connectors/internal/api/bank_account_test.go(0 hunks)cmd/connectors/internal/api/connector.go(0 hunks)cmd/connectors/internal/api/connector_test.go(0 hunks)cmd/connectors/internal/api/connectorconfigs.go(0 hunks)cmd/connectors/internal/api/connectormodule.go(0 hunks)cmd/connectors/internal/api/connectors_manager/connector_test.go(0 hunks)cmd/connectors/internal/api/connectors_manager/errors.go(0 hunks)cmd/connectors/internal/api/connectors_manager/loader.go(0 hunks)cmd/connectors/internal/api/connectors_manager/manager.go(0 hunks)cmd/connectors/internal/api/connectors_manager/manager_test.go(0 hunks)cmd/connectors/internal/api/connectors_manager/store.go(0 hunks)
⛔ Files not processed due to max files limit (29)
- cmd/connectors/internal/api/connectors_manager/storememory_test.go
- cmd/connectors/internal/api/connectors_manager/taskscheduler.go
- cmd/connectors/internal/api/health.go
- cmd/connectors/internal/api/module.go
- cmd/connectors/internal/api/read_connectors.go
- cmd/connectors/internal/api/read_connectors_test.go
- cmd/connectors/internal/api/recovery.go
- cmd/connectors/internal/api/router.go
- cmd/connectors/internal/api/service/bank_account.go
- cmd/connectors/internal/api/service/bank_account_test.go
- cmd/connectors/internal/api/service/connector.go
- cmd/connectors/internal/api/service/errors.go
- cmd/connectors/internal/api/service/ping.go
- cmd/connectors/internal/api/service/service.go
- cmd/connectors/internal/api/service/service_test.go
- cmd/connectors/internal/api/service/transfer_initiation.go
- cmd/connectors/internal/api/service/transfer_initiation_test.go
- cmd/connectors/internal/api/service/transfer_reversal.go
- cmd/connectors/internal/api/transfer_initiation.go
- cmd/connectors/internal/api/transfer_initiation_test.go
- cmd/connectors/internal/api/transfer_reversal.go
- cmd/connectors/internal/connectors/adyen/client/accounts.go
- cmd/connectors/internal/connectors/adyen/client/client.go
- cmd/connectors/internal/connectors/adyen/client/webhooks.go
- cmd/connectors/internal/connectors/adyen/config.go
- cmd/connectors/internal/connectors/adyen/connector.go
- cmd/connectors/internal/connectors/adyen/currencies.go
- cmd/connectors/internal/connectors/adyen/errors.go
- cmd/connectors/internal/connectors/adyen/loader.go
💤 Files with no reviewable changes (74)
- cmd/api/internal/storage/utils.go
- cmd/api/internal/api/service/ping.go
- cmd/api/internal/storage/ping.go
- cmd/api/internal/storage/connectors.go
- cmd/api/internal/storage/accounts_test.go
- cmd/api/internal/api/service/balance.go
- cmd/api/root.go
- cmd/api/internal/api/api_utils_test.go
- cmd/api/internal/api/metadata.go
- cmd/api/internal/api/recovery.go
- cmd/api/internal/storage/connectors_test.go
- cmd/api/internal/storage/transfer_initiation_test.go
- cmd/api/internal/storage/module.go
- cmd/api/internal/storage/metadata.go
- cmd/api/internal/storage/main_test.go
- cmd/connectors/internal/api/connectorconfigs.go
- cmd/api/internal/api/service/transfer_initiations.go
- cmd/api/internal/storage/bank_accounts_test.go
- cmd/api/internal/api/router.go
- cmd/api/internal/api/service/errors.go
- cmd/api/internal/storage/repository.go
- cmd/api/internal/storage/balances_test.go
- cmd/api/internal/api/health.go
- cmd/api/internal/api/metadata_test.go
- cmd/connectors/internal/api/backend/backend_generated.go
- cmd/api/internal/api/bank_accounts_test.go
- cmd/connectors/internal/api/connectors_manager/store.go
- cmd/api/internal/api/transfer_initiation_test.go
- cmd/api/internal/api/accounts_test.go
- cmd/connectors/internal/api/api_utils_test.go
- cmd/api/internal/api/service/bank_accounts.go
- cmd/api/internal/api/module.go
- cmd/api/internal/storage/payments_test.go
- cmd/api/internal/api/service/payments_test.go
- cmd/api/serve.go
- cmd/api/internal/storage/sort.go
- cmd/api/internal/api/payments_test.go
- cmd/api/internal/api/utils.go
- cmd/api/internal/api/balances_test.go
- cmd/connectors/internal/api/connectors_manager/manager_test.go
- cmd/api/internal/api/transfer_initiation.go
- cmd/api/internal/api/service/accounts_test.go
- cmd/api/internal/api/balances.go
- cmd/connectors/internal/api/connectormodule.go
- cmd/api/internal/api/service/accounts.go
- cmd/api/internal/storage/bank_accounts.go
- cmd/api/internal/api/payments.go
- cmd/connectors/internal/api/connectors_manager/errors.go
- cmd/connectors/internal/api/bank_account_test.go
- cmd/connectors/internal/api/connector_test.go
- cmd/api/internal/storage/pools_test.go
- cmd/api/internal/api/bank_accounts.go
- cmd/api/internal/api/pools_test.go
- cmd/connectors/internal/api/bank_account.go
- cmd/api/internal/storage/pools.go
- cmd/api/internal/storage/transfer_initiation.go
- cmd/api/internal/api/backend/backend.go
- cmd/connectors/internal/api/backend/backend.go
- cmd/api/internal/api/service/pools_test.go
- cmd/api/internal/storage/paginate.go
- cmd/api/internal/api/accounts.go
- cmd/connectors/internal/api/connectors_manager/loader.go
- cmd/api/internal/storage/accounts.go
- cmd/api/internal/storage/payments.go
- cmd/api/internal/api/service/payments.go
- cmd/api/internal/api/service/service.go
- cmd/connectors/internal/api/connector.go
- cmd/api/internal/api/service/pools.go
- cmd/api/internal/api/pools.go
- cmd/connectors/internal/api/connectors_manager/connector_test.go
- cmd/connectors/internal/api/connectors_manager/manager.go
- cmd/api/internal/storage/balances.go
- cmd/api/internal/api/backend/backend_generated.go
- cmd/api/internal/api/service/service_test.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (2)
Earthfile (1)
124-124: Ensure the test directory is copied to the correct location
The command COPY --dir test . copies the test directory to the current working directory, which might not be intended if the working directory is /src. This could lead to the tests not being properly included in the tidy process.
Confirm that the test directory is being copied to /src as intended:
COPY --dir test .Modify to:
COPY --dir test /srcThis change ensures that the test directory is placed within /src, consistent with other source files.
CODEOWNERS (1)
1-1: Confirm CODEOWNERS syntax and team permissions
The addition of * @formancehq/backend assigns ownership of all files to the @formancehq/backend team. Ensure that:
- The syntax is correct and matches the CODEOWNERS file format.
- The
@formancehq/backendteam exists and has the appropriate permissions in the repository. - This change aligns with the project's contribution and review workflow.
You can verify the CODEOWNERS syntax by consulting the GitHub CODEOWNERS documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Earthfile (3)
8-10: Add essential PostgreSQL configurations and health checkThe postgres service definition should include:
- Environment variables for security (POSTGRES_PASSWORD, etc.)
- Health check for reliability
- Volume mount for data persistence
Apply this diff:
postgres: FROM postgres:15-alpine + ENV POSTGRES_PASSWORD=local_password + ENV POSTGRES_DB=payments + HEALTHCHECK --interval=10s --timeout=5s --retries=5 \ + CMD pg_isready -U postgres -d payments + VOLUME ["/var/lib/postgresql/data"]
64-79: Consider adding test timeout configurationThe test configuration looks good with proper separation of integration and unit tests. Consider adding a timeout configuration to prevent long-running tests from blocking the pipeline.
Add timeout to the test command:
- RUN go test $goFlags ./... + RUN go test -timeout=10m $goFlags ./...
131-145: Remove or document commented codeThe commented
generate-generic-connector-clientsection should either be:
- Removed if it's no longer needed
- Documented with a clear explanation if it's kept for reference
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Earthfile(3 hunks)
🔇 Additional comments (5)
Earthfile (5)
15-15: LGTM!
The order of copying source directories is logical and maintains consistency with the project structure.
32-37: LGTM! Great improvement in JSON handling
The use of jq for merging configurations is much more robust than the previous approach of concatenating JSON fragments.
42-43: LGTM!
The addition of generated files to the compilation step is properly ordered and maintains dependencies.
53-55: LGTM! Intentional failure on missing plugins
As previously discussed, the current implementation correctly fails the build if no plugins are present, which is the desired behavior.
122-130: LGTM!
The simplified generation process with proper tool installation is clean and maintainable.
| compile-plugins: | ||
| FROM core+builder-image | ||
| COPY (+sources/*) /src | ||
| COPY (+compile-configs/configs.json) /src/internal/connectors/plugins/configs.json | ||
| WORKDIR /src/internal/connectors/plugins/public | ||
| RUN printf "package public\n\n" > list.go | ||
| RUN printf "import (\n" >> list.go | ||
| FOR c IN $(ls -d */ | sed 's#/##') | ||
| RUN printf " _ \"github.com/formancehq/payments/internal/connectors/plugins/public/$c\"\n" >> list.go | ||
| END | ||
| RUN printf ")\n" >> list.go | ||
| SAVE ARTIFACT /src/internal/connectors/plugins/public/list.go /list.go |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using go:generate or templates for plugin list generation
The current approach of using shell commands for Go code generation is error-prone and harder to maintain. Consider:
- Using go:generate with a custom tool
- Using Go templates
- Adding validation for plugin directory structure
Example implementation using Go templates:
compile-plugins:
FROM core+builder-image
COPY (+sources/*) /src
COPY (+compile-configs/configs.json) /src/internal/connectors/plugins/configs.json
WORKDIR /src/internal/connectors/plugins/public
- RUN printf "package public\n\n" > list.go
- RUN printf "import (\n" >> list.go
- FOR c IN $(ls -d */ | sed 's#/##')
- RUN printf " _ \"github.com/formancehq/payments/internal/connectors/plugins/public/$c\"\n" >> list.go
- END
- RUN printf ")\n" >> list.go
+ COPY templates/list.go.tmpl .
+ RUN go run tools/gen-plugin-list/main.go -template list.go.tmpl -output list.go
SAVE ARTIFACT /src/internal/connectors/plugins/public/list.go /list.goCommittable suggestion skipped: line range outside the PR's diff.
No description provided.