Implement validation for Stellar deposits #86
Conversation
| // contract | ||
| Amount string `db:"amount"` | ||
| // LedgerTime is the unix timestamp of the deposit | ||
| LedgerTime int64 `db:"ledger_time"` |
There was a problem hiding this comment.
I had some issues storing time.Time values in the db in the integration tests which is why I decided to just store the unix timestamp instead
There was a problem hiding this comment.
I used it starting at https://github.com/stellar/starbridge/tree/d0314d3e51b7205ec8aaae278825765dc943d46c for outgoing_stellar_transactions.expiration. The type was timestamp without time zone and it was working fine.
| StellarTxNotSuccessful = problem.P{ | ||
| Type: "stellar_tx_not_successful", | ||
| Title: "Stellar Transaction Not Successful", | ||
| Status: http.StatusUnprocessableEntity, | ||
| Detail: "The stellar transaction was not successful.", | ||
| } | ||
| StellarTxHasMultipleOperations = problem.P{ | ||
| Type: "stellar_tx_has_multiple_operations", | ||
| Title: "Stellar Transaction Has Multiple Operations", | ||
| Status: http.StatusUnprocessableEntity, | ||
| Detail: "The stellar deposit transaction is invalid because it has multiple operations.", | ||
| } | ||
| StellarTxIsNotPayment = problem.P{ | ||
| Type: "stellar_tx_is_not_payment", | ||
| Title: "Stellar Transaction Is Not Payment", | ||
| Status: http.StatusUnprocessableEntity, | ||
| Detail: "The stellar deposit transaction is invalid because it is not a payment.", | ||
| } | ||
| StellarTxIsNotPaymentToBridge = problem.P{ | ||
| Type: "stellar_tx_is_not_payment_to_bridge", | ||
| Title: "Stellar Transaction Is Not Payment To Bridge", | ||
| Status: http.StatusUnprocessableEntity, | ||
| Detail: "The stellar deposit transaction is invalid because it " + | ||
| "is not a payment to the bridge account.", | ||
| } |
There was a problem hiding this comment.
I wonder if we can merge all these errors into stellar_tx_invalid and put details in Detail field. Unless we need to be able to check the code somewhere other than frontend it would make sense.
| return store.StellarDeposit{}, err | ||
| } | ||
|
|
||
| ops, err := client.Operations(horizonclient.OperationRequest{ |
There was a problem hiding this comment.
I think it can be a problem for Horizons with partial history. Maybe we should store the incoming payments in a DB (as we do for withdrawals in stellar.Observer)? I guess we'd need to introduce a minimum deposit amount to prevent spamming the account with stroop payments.
There was a problem hiding this comment.
I think that makes sense since we are already processing incoming payments
Co-authored-by: Bartek Nowotarski <bartek@nowotarski.info>
|
@bartekn I implemented your suggestion of ingesting both incoming and outgoing payments to the bridge, PTAL |
stellar/txobserver/main.go
Outdated
| } | ||
| // Skip inserting transactions with multiple ops. Currently Starbridge | ||
| // does not create such transactions but it can change in the future. | ||
| return len(tx.Operations()) == 1 |
There was a problem hiding this comment.
Because ingestIncomingPayment also should have a memo validation, maybe we could move:
payment.Transaction.MemoType != "hash" || payment.Transaction.Memo == ""check here from ingestOutgoingPayment?
There was a problem hiding this comment.
The reason I chose to not validate the transaction memo for incoming payments is that validation of those fields should only be implemented in the HTTP handler for the withdrawal endpoint. If a user does not set the memo correctly on their payment to the bridge then the withdrawal should fail, however, it should still be possible to refund the payment. This will allow people who didn't set the memo properly to still be able to recover their deposit via refunds.
| assetString = "native" | ||
| } else { | ||
| assetString = payment.Asset.Code + ":" + payment.Asset.Issuer | ||
| } |
There was a problem hiding this comment.
We should also validate memo here. Check the previous comment.
| LedgerTime: payment.LedgerCloseTime.Unix(), | ||
| Sender: payment.From, | ||
| Destination: payment.Transaction.Memo, | ||
| Amount: payment.Amount, |
There was a problem hiding this comment.
Can be done later (after MVP) but we should probably add a config value for minimum deposit. Otherwise it's possible to add thousands of rows in Starbridge DB quite easily. EDIT: created #88.
Co-authored-by: Bartek Nowotarski <bartek@nowotarski.info>
This PR implements getStellarDeposit() which is a helper function to extract and validate a deposit to the Stellar bridge account from an http request. getStellarDeposit() will be used by the HTTP handlers for the Stellar -> Ethereum withdrawal and refund endpoints.
A stellar deposit is valid if:
Note that we do not validate the Stellar asset or the ethereum address of the recipient (which is encoded in the transaction memo). Validation of those fields should only be implemented in the HTTP handler for the withdrawal endpoint. If a user does not set the memo correctly on their payment to the bridge then the withdrawal should fail, however, it should still be possible to refund the payment.