Skip to content

SDP 1644 create direct payment #693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

dpohr
Copy link
Collaborator

@dpohr dpohr commented May 30, 2025

What

Implements POST /payments API endpoint for direct payments to existing receivers without creating disbursements. Adds new "DIRECT" payment type with flexible entity reference system (by ID, email, phone, wallet address) and integrates with existing payment processing pipeline.

Why

Enables on-demand, single payments for API integrations that don't fit the batch disbursement workflow. Critical for use cases like instant payments, manual corrections, and external system integrations that need immediate payment capability without disbursement overhead.

Known limitations

  • Existing receivers only - does not create new receivers (by design for this iteration)
  • Contract/Fiat assets - marked as future implementation, returns proper "not supported" errors
  • Legacy auth - still uses token extraction method (TODO: migrate to API keys when available)

Checklist

  • Title follows SDP-1234: Add new feature or Chore: Refactor package xyz format. The Jira ticket code was included if available.
  • PR has a focused scope and doesn't mix features with refactoring
  • Tests are included (if applicable)
  • CHANGELOG.md is updated (if applicable)
  • CONFIG/SECRETS changes are updated in helmcharts and deployments (if applicable)
  • Preview deployment works as expected
  • Ready for production

@dpohr dpohr temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) May 30, 2025 06:51 — with GitHub Actions Inactive
@dpohr dpohr temporarily deployed to Anchor Integration Tests May 30, 2025 06:51 — with GitHub Actions Inactive
@dpohr dpohr temporarily deployed to Anchor Integration Tests May 30, 2025 06:51 — with GitHub Actions Inactive
@dpohr dpohr temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) May 30, 2025 06:51 — with GitHub Actions Inactive
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@stellar-jenkins
Copy link

@dpohr dpohr temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) May 30, 2025 07:23 — with GitHub Actions Inactive
@dpohr dpohr temporarily deployed to Anchor Integration Tests May 30, 2025 07:23 — with GitHub Actions Inactive
@stellar-jenkins
Copy link

@dpohr dpohr temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) May 30, 2025 07:52 — with GitHub Actions Inactive
@dpohr dpohr temporarily deployed to Anchor Integration Tests May 30, 2025 07:52 — with GitHub Actions Inactive
@stellar-jenkins
Copy link

@dpohr dpohr mentioned this pull request May 30, 2025
7 tasks
@dpohr dpohr requested a review from marwen-abid June 2, 2025 09:17
@dpohr dpohr temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) June 2, 2025 13:02 — with GitHub Actions Inactive
@dpohr dpohr temporarily deployed to Anchor Integration Tests June 2, 2025 13:02 — with GitHub Actions Inactive
@stellar-jenkins
Copy link

@marwen-abid marwen-abid requested a review from philipliu June 3, 2025 18:01
@dpohr dpohr marked this pull request as ready for review June 4, 2025 10:52
asset_id,
receiver_id,
disbursement_id,
receiver_wallet_id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formatting

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this keep happening since the default golang linter uses tabs, but current codebase uses spaces, this is not regular choice. I'll fix it, however we need to consider reformat files to be consistent with go style

insert.Amount,
insert.AssetID,
insert.ReceiverID,
insert.DisbursementID, // Will be NULL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If DisbursementID is null, can we just exclude it from the query?

errToMatch = err
}

switch e := errToMatch.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using errors.Is instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, but it's better to use .As() then, will fix it, thank you

httperror.BadRequest(e.Error(), err, nil).Render(w)

default:
if strings.Contains(err.Error(), "receiver wallet not found") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matching against the receiver wallet not found string seems brittle. Is there a way to avoid this?

err = unwrapTransactionError(err)
var walletErr WalletNotEnabledError
assert.True(t, errors.As(err, &walletErr))
assert.Contains(t, err.Error(), "Calth Reserve")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we replace this with assert.Contains(t, err.Error(), &disabledWallet.Name), so it's clear that this is the disabled wallet?

var assetErr AssetNotSupportedByWalletError
assert.True(t, errors.As(err, &assetErr))
assert.Contains(t, err.Error(), "XLM")
assert.Contains(t, err.Error(), "Macragge Treasury")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we replace Macragge Treasury with &enabledWallet.Name?

return fmt.Errorf("parsing amount: %w", err)
}

// Skip balance validation for user-managed wallets since they manage their own balance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also skip this validation for the regular disbursement flow? I'm wondering why we skip the validation if the account is managed outside of SDP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, can we refer to this as something other than "user-managed wallet" in both this comment and the test cases? It sounds like we are validating the balances of the receiver wallets, otherwise.

Comment on lines +227 to +239
// Create user-managed wallet
userManagedWallet := data.CreateWalletFixture(t, ctx, dbConnectionPool, "User Wallet", "stellar.org", "stellar.org", "stellar://")
data.MakeWalletUserManaged(t, ctx, dbConnectionPool, userManagedWallet.ID)

// Create receiver wallet with specific address
rw := data.CreateReceiverWalletFixture(t, ctx, dbConnectionPool, receiver.ID, userManagedWallet.ID, data.RegisteredReceiversWalletStatus)

// Update with the specific stellar address
stellarAddress := "GBXGQJWVLWOYHFLVTKWV5FGHA3LNYY2JQKM7OAJAUEQFU6LPCSEFVXON"
err := models.ReceiverWallet.Update(ctx, rw.ID, data.ReceiverWalletUpdate{
StellarAddress: stellarAddress,
}, dbConnectionPool)
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought "user-managed wallet" refers to the distribution account being managed outside of SDP. Does the receiver wallet, which is also user-managed, affect the test?

expectedAmount: 50.00,
},
{
name: "terminal and draft statuses ignored",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this test be merged with sum for all in-progress statuses?

Copy link
Collaborator

@marwen-abid marwen-abid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of great stuff in this PR. This is my first stab at a review, will add more comments as I test this more.

ADD CONSTRAINT check_payment_type_disbursement
CHECK (
(payment_type = 'DISBURSEMENT' AND disbursement_id IS NOT NULL) OR
(payment_type = 'DIRECT' AND disbursement_id IS NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! Thank you for adding this check

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name of this file has copy at the end. Also please make sure when merging these PRs that the date in the name of the file remains chronological.

"github.com/stellar/stellar-disbursement-platform-backend/stellar-auth/pkg/auth"
)

// DirectPaymentRequest represents service-level request for creating direct payment
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// DirectPaymentRequest represents service-level request for creating direct payment
// CreateDirectPaymentRequest represents service-level request for creating direct payment.

func (s *DirectPaymentService) CreateDirectPayment(
ctx context.Context,
req CreateDirectPaymentRequest,
user *auth.User,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨Doesn't seem like user is being used in the code. We should probably add a field to the payments table to keep track of the user that submits the payment in the context of Direct Payments.

If you want to tackle that later, you can start by adding the user_id to the status_message field in status_history. E.g. Payment submitted by user_id xxx .


// DirectPaymentRequest represents service-level request for creating direct payment
type CreateDirectPaymentRequest struct {
Amount string `json:"amount" validate:"required"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓For the tagvalidate:"required", what validator is being used ? I don't see one called.

httperror.BadRequest("invalid reference in request", err, nil).Render(w)

case services.NotFoundError:
httperror.NotFound("resource not found", err, nil).Render(w)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should improve the error being returned here. It currently doesn't specify which resource was not found.
Request

{
    "asset": {
        "code": "USDC",
        "issuer": "GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5",
        "type": "classic"
    },
    "amount": "10.44",
    "receiver": {
        "email": "[email protected]"
    }
}

Response:

{
    "error": "resource not found"
}

Logs :

receiver not found: no receiver found with contact info: [email protected]"

}

// Skip balance validation for user-managed wallets since they manage their own balance
if distributionAccount.Type != schema.DistributionAccountStellarDBVault {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this check is necessary. We are able and we should check the balance for other distribution account types like DistributionAccountStellarEnv or DistributionAccountCircleDBVault

// Get available balance for vault-managed accounts
availableBalance, err := s.DistributionAccountService.GetBalance(ctx, distributionAccount, *asset)
if err != nil {
return fmt.Errorf("getting balance: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the distribution account doesn't have a trustline for the target asset, this balance check causes the API call to fail with 500

{
    "error": "creating payment"
}

Logs are:

level=error msg="[DRY_RUN Crash Reporter] creating payment: transaction execution error: getting balance: balance for asset {722eb29d-5133-4e5a-8780-8eb3adb3d59c USDC GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5 2024-07-26 23:45:48.617281 +0000 UTC 2024-07-26 23:45:48.617281 +0000 UTC <nil>} not found for distribution account"

This is not the desired behavior. We should do one of two things:

  1. Validate that the distribution account has a trustline for the target asset.
  2. Or, Return an error that informs the end-user about why this balance check failed so that they can know to either change the asset, or add a trustline to their distribution account.

return nil, fmt.Errorf("creating payment: %w", err)
}

// 7. Update payment status based on receiver wallet status
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 This is not needed. DRAFT Payment doesn't exist in the context of Direct Payments, it only exists in the context of Disbursements. Furthermore, if you set the payment to Draft, it will get stuck in that state even after the Wallet is registered.

For Direct Payment, the lifecycle of a payment should start with READY status. Once the wallet is REGISTERED, The payment will be processed and moved to the PENDING state automatically.

@@ -221,7 +250,7 @@ func (p *PaymentModel) GetBatchForUpdate(ctx context.Context, sqlExec db.SQLExec
}

query := getReadyPaymentsBaseQuery + `
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 I haven't been able to dispatch a payment (using schedule jobs), because getReadyPaymentsBaseQuery still checks that the disbursemen.status = 'STARTED'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants