-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: develop
Are you sure you want to change the base?
Conversation
Something went wrong with PR preview build please check |
stellar-disbursement-platform-backend-preview is available here: |
stellar-disbursement-platform-backend-preview is available here: |
stellar-disbursement-platform-backend-preview is available here: |
stellar-disbursement-platform-backend-preview is available here: |
asset_id, | ||
receiver_id, | ||
disbursement_id, | ||
receiver_wallet_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: formatting
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
// 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) |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
?
There was a problem hiding this 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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, |
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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:
- Validate that the distribution account has a trustline for the target asset.
- 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 |
There was a problem hiding this comment.
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 + ` |
There was a problem hiding this comment.
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'
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
Checklist
SDP-1234: Add new feature
orChore: Refactor package xyz
format. The Jira ticket code was included if available.CHANGELOG.md
is updated (if applicable)