Feat/trades and bitstamp #606
Conversation
feat: adding a connector development server with direct access to connectors functions Signed-off-by: Clément Salaün <[email protected]>
Signed-off-by: Clément Salaün <[email protected]>
* Handle SQLSTATE 22P02 in storage as HTTP 400 * Disable profiler in e2e tests to remove unneeded overhead
…ling - Added detailed logging for account and balance fetching processes. - Refactored payment transaction handling to support multiple payment types from a single transaction. - Updated transaction struct to dynamically capture exchange rates. - Cleaned up unused comments and improved code readability across various files.
…validation for trade legs
…ndling - Introduced FetchNextTrades method in the Bitstamp plugin to retrieve trades. - Updated fetchNextTrades to handle unique trades and filter out duplicates. - Modified transactionToTrade to include account reference and adjusted logging. - Removed legacy payment handling for exchange transactions, now managed by FetchTrades. - Added new capability for fetching trades in the Bitstamp capabilities list.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
| @@ -0,0 +1,185 @@ | |||
| package main | |||
There was a problem hiding this comment.
Do we still need this file for something?
| return e("failed to convert trade to model", err) | ||
| } | ||
|
|
||
| payload := internalEvents.TradeMessagePayload{ |
There was a problem hiding this comment.
Does the consumer of the event need the entire payload? It might be an option to send fewer fields and allow the consumer to query payments if they want additional information.
There was a problem hiding this comment.
Yes we could indeed! What would be the gain of doing that though?
There was a problem hiding this comment.
Some queue systems have somewhat limited payload size so if you have big nested structures you might run into the upper limits and not be able to send the event at all.
But also, just in general I'd be interested to know what the business use-case of these notifications would be.
What value does it bring to clients to know about new trades? What will they typically do with that info?
Fabrice recently did a big refactor of the notifications to reduce the operational costs, so it should be cheaper for us to send notifications now, but I also wanted to check if you added this function to send notifications because there was a particular business use-case in mind, or if you just copied the pattern we had for payments?
| CreateFormancePayment(ctx context.Context, payment models.Payment) error | ||
| // Create a Formance trade, no call to the plugin, just a creation | ||
| // of a trade in the database related to the provided connector id. | ||
| CreateFormanceTrade(ctx context.Context, trade models.Trade) error |
There was a problem hiding this comment.
What is the purpose of creating a trade on Formance only? If it doesn't connect to the PSP then what is it used for?
For payments we have a create payment endpoint which is used primarily by people using the generic connector that need a way to backfill data that for one reason or another their endpoint won't return via the connector.
But we have two separate entities:
- payment_initiation (created via formance API)
- payment (imported via the connector)
the former is the intent to create a payment, whereas the latter is a representation of something that exists on the PSP. The payment_initiation can be forwarded to the PSP - this will result in a corresponding payment being linked.
Here you have a mixed trade object that may or may not represent a real trade known by the PSP. You also allow (local) updates of the trade metadata which will never be reflected on the PSP.
Would it not be better to keep two different entities that can be linked, like we do for payments?
| } | ||
|
|
||
| if len(nextTasks) > 0 { | ||
| // Logic for next tasks if needed |
There was a problem hiding this comment.
You're probably not seeing any issues with this remaining unimplemented because your connector put fetch trades as the last element in the plugin workflow. Other connectors might not have trades being the last element, which will result in the rest of the workflow never executing. Let's handle this properly.
| // Safely get logger - it may be nil in unit tests | ||
| var logger interface{ Info(string, ...interface{}); Error(string, ...interface{}) } | ||
| // Try to get activity logger, but don't panic if not in activity context | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Not an activity context, logger stays nil | ||
| logger = nil | ||
| } | ||
| }() | ||
| logger = activity.GetLogger(ctx) | ||
| }() | ||
|
|
||
| if logger != nil { | ||
| logger.Info("SendEvents activity started", | ||
| "idempotency_key", req.IdempotencyKey, | ||
| "has_payment", req.Payment != nil) | ||
| } | ||
|
|
There was a problem hiding this comment.
You can actually just use a.logger which can be set in unit tests as well. Then we don't need all these if statements checking if the logger is nil
| // TODO: accountsState will be used to know at what point we're at when | ||
| // fetching the PSP accounts. | ||
| // This struct will be stored as a raw json, you're free to put whatever | ||
| // you want. | ||
| // Example: | ||
| // LastPage int `json:"lastPage"` | ||
| // LastIDCreated int64 `json:"lastIDCreated"` |
There was a problem hiding this comment.
Pagination is unimplemented?
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| // Try to unmarshal error response | ||
| body, _ := io.ReadAll(resp.Body) |
| func newNonce() string { | ||
| const charset = "abcdefghijklmnopqrstuvwxyz0123456789" | ||
| b := make([]byte, 36) | ||
| rand.Read(b) | ||
| for i := range b { | ||
| b[i] = charset[b[i]%byte(len(charset))] | ||
| } | ||
| return string(b) | ||
| } |
There was a problem hiding this comment.
Since this is only pseudorandom, there might be collisions if a client has multiple payments pods running. Probably not a critical issue right now, but probably using the current time as part of the random generation would decrease chances of collision.
| } | ||
|
|
||
| // ExpectedLegAmounts calculates expected BASE and QUOTE payment amounts | ||
| func ExpectedLegAmounts(trade Trade) (base decimal.Decimal, quote decimal.Decimal) { |
There was a problem hiding this comment.
Any reason you're using this particular decimal library instead of handling decimals the way we do for amounts in payments / balance?
Summary by cubic
Add first-class Trades support across the API and engine, and introduce a Bitstamp connector with trade ingestion. Also adds an initial Coinbase Prime connector with accounts, balances, payments, and wallet-to-wallet transfers.
New Features
Dependencies
Written for commit a987f9b. Summary will update automatically on new commits.