-
Notifications
You must be signed in to change notification settings - Fork 1
feat: API for instant batch #16
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
Conversation
Summary of ChangesHello @martinserts, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new API endpoint Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new API endpoint /v1/payment-batches for creating instant payment batches. The implementation is well-structured, including robust validation and idempotency checks. The changes also include a good refactoring of the MAX_BATCH_SIZE constant for better code organization.
My review includes two main points:
- A high-severity fix in the database layer to prevent a potential server panic caused by an
.unwrap(). - A medium-severity suggestion to improve the maintainability and readability of the new API handler by making state synchronization more explicit.
Overall, this is a solid contribution that adds valuable functionality.
| payment_ids_for_batch.push(new_payment.id.clone()); | ||
| created_payments.push(new_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.
I might be misinterpreting the purpose here so please correct me.
I think the idempotentency key exists to ensure that multiple similar requests that come in aren't processed multiple times? But if we recreate a new key every time a request comes in we're not really performing any kind of check to prevent re-processing the same batch? It does look like we check for existing payments based entirely on the list of payees which makes the idempotency key superfluous and could also make for false positives. You should be able to create multiple transactions to the same group of addresses, i would think.
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 might be misinterpreting the purpose here so please correct me.
I think the idempotentency key exists to ensure that multiple similar requests that come in aren't processed multiple times? But if we recreate a new key every time a request comes in we're not really performing any kind of check to prevent re-processing the same batch? It does look like we check for existing payments based entirely on the list of payees which makes the idempotency key superfluous and could also make for false positives. You should be able to create multiple transactions to the same group of addresses, i would think.
You are right regarding idempotency, but there is a distinction here between the incoming request and the outgoing operation.
There are actually two different idempotency keys in this flow:
client_id(Incoming): This is the key provided by the API consumer. We use this to prevent processing the same request from our clients twice. This is stored inpaymentstable for each payment.pr_idempotency_key(Outgoing): This is the key we generate on line 267. We use this when acting as a client to the internalminatari_cli(Payment Receiver) (via theaccounts_api::api_lock_fundscall). This is stored inpayment_batchestable.
I think the idempotentency key exists to ensure that multiple similar requests that come in aren't processed multiple times?
Correct. The check for incoming duplicate requests happens before line 267.
In api_create_payment_batch, we extract the client_id from every item in the incoming request into item_client_ids. We then query the DB:
let existing_payments = Payment::find_by_client_ids(&mut tx, &item_client_ids, &request.account_name).await?;- If these IDs exist: that means this as a duplicate request. We return the existing batch immediately and never reach line 267 (so a new UUID is not generated).
- If these IDs do not exist: We proceed to create new records.
It does look like we check for existing payments based entirely on the list of payees... You should be able to create multiple transactions to the same group of addresses.
We actually check based on the client_id (the unique string provided by the user for each payment), not the recipient address.
If you send a request to pay Bob 10 tari with client_id: "A", and then send a second request to pay Bob 10 tari with client_id: "B", the system sees "B" does not exist and allows the second payment.
But if we recreate a new key every time a request comes in...
Line 267 (Uuid::new_v4()) is only reached once we have confirmed this is a new, non-duplicate request.
We generate this UUID to ensure that if our worker crashes while talking to the minatari_cli (Payment Receiver), we can retry the lock_funds call safely using this stored UUID (pr_idempotency_key).
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 actually check based on the client_id (the unique string provided by the user for each payment), not the recipient address.
I feel like this clears it up the most. I think I was confusing the two and thought that client_id was the recipient address.
Thanks!
Description
Adds
/v1/payment-batchesAPI endpoint to create instant batch.It may contain up to 100 recipients, which will be included in the same transaction (batch).
This batch is created instantly, and will not be part of batching worker.
Closes: #8
Motivation and Context
Up until now, users could create payments specifying amount and recipient.
After that all newly created payments where processed by batching worker, which gathered them into batches on certain invervals.
This change allows to create instant batches.
How Has This Been Tested?
Invalid account name
{ "error": "Account 'unknown' not found in configuration" }Empty batch
{ "error": "Batch cannot be empty" }Successful instant batch
{ "batch_id": "1e942c3e-f1fb-4e2e-a52a-57b2297d37dd", "account_name": "default", "status": "PENDING_BATCHING", "payments": [ { "payment_id": "60e14dc6-ae5e-4b8c-94db-db2607e9821f", "status": "BATCHED", "client_id": "first", "account_name": "default", "recipient_address": "f23tYJkf4QZfCS4kGSXJug1wUokDN3gEixfN6XXYpjUfhsKwugBcNU8mrRoSv82gMv72wbH79DAp3LvGdsCPSgCkE5B", "amount": 1000001, "created_at": "2025-12-16T12:57:23Z", "updated_at": "2025-12-16T12:57:23Z" }, { "payment_id": "fa51e062-3af7-47d5-bb2e-3b3cb900cf50", "status": "BATCHED", "client_id": "second", "account_name": "default", "recipient_address": "f2MWPuubAoeJyhgeNqaPTG4xXeo4rjtSFWLcXysboCwC2ZAJuD4pb7Ldkd9BCFACf77K4RiEufCi7e6D52fQEnTR3CA", "amount": 1000002, "created_at": "2025-12-16T12:57:23Z", "updated_at": "2025-12-16T12:57:23Z" }, { "payment_id": "8a96fdb1-e4d8-4079-acf8-ded819c74912", "status": "BATCHED", "client_id": "third", "account_name": "default", "recipient_address": "f2LUUip8F3LCQ6PyJvQiySCEpQyzTQSfvQF4YX3N9pY4rz4gAUprFgaL9JANYinYN2HGtKexmfX2odeNHfatoNhozaL", "amount": 1000003, "created_at": "2025-12-16T12:57:23Z", "updated_at": "2025-12-16T12:57:23Z" } ] }Idempotency
Posted the request (that is above AGAIN), it returned the same and did not create new batch.
What process can a PR reviewer use to test or verify this change?
Breaking Changes