Fix ba consume #126
Fix ba consume #126
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| interface LockAmountDTO { | ||
| challengeId: string; | ||
| lockAmount: number; | ||
| amount: number; |
There was a problem hiding this comment.
[maintainability]
The renaming of lockAmount to amount could lead to confusion if the context of its usage is not clear. Ensure that the change is consistently applied throughout the codebase and that the new name accurately reflects its purpose in all contexts.
| challengeId: string; | ||
| consumeAmount: number; | ||
| markup?: number; | ||
| amount: number; |
There was a problem hiding this comment.
[💡 readability]
The renaming of consumeAmount to amount might reduce clarity if the distinction between different types of amounts is important. Consider whether a more descriptive name would better convey the intended use in this context.
| { | ||
| method: 'PATCH', | ||
| body: JSON.stringify({ param: dto }), | ||
| body: JSON.stringify(dto), |
There was a problem hiding this comment.
[❗❗ correctness]
Changing JSON.stringify({ param: dto }) to JSON.stringify(dto) modifies the structure of the request body. Ensure that the API endpoint expects the new format, as this change could lead to unexpected behavior if the API expects the param wrapper.
| ); | ||
| throw new Error( | ||
| `Budget Error: Requested amount $${dto.lockAmount} exceeds available budget for Billing Account #${billingAccountId}. | ||
| `Budget Error: Requested amount $${dto.amount} exceeds available budget for Billing Account #${billingAccountId}. |
There was a problem hiding this comment.
[❗❗ correctness]
The variable dto.amount is used here, but it is not clear from the diff whether this change is intentional or if dto.lockAmount was the correct variable to use. Ensure that dto.amount is indeed the intended value for this error message, as using the wrong variable could lead to incorrect error reporting.
| { | ||
| method: 'PATCH', | ||
| body: JSON.stringify({ param: dto }), | ||
| body: JSON.stringify(dto), |
There was a problem hiding this comment.
[❗❗ correctness]
The change from JSON.stringify({ param: dto }) to JSON.stringify(dto) alters the structure of the request payload. Ensure that the API endpoint expects the new payload format, as this could affect the correctness of the request.
| await this.lockAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, | ||
| lockAmount: | ||
| amount: |
There was a problem hiding this comment.
[❗❗ correctness]
The property name change from lockAmount to amount should be verified for consistency across the codebase. Ensure that this change does not break any other parts of the application that rely on the previous property name.
|
|
||
| if (currAmount !== prevAmount) { | ||
| await this.consumeAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, |
There was a problem hiding this comment.
[❗❗ correctness]
The property name change from consumeAmount to amount should be verified for consistency across the codebase. Ensure that any other references to consumeAmount are updated accordingly to prevent runtime errors.
| await this.lockAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, | ||
| lockAmount: rollback ? prevAmount : 0, | ||
| amount: rollback ? prevAmount : 0, |
There was a problem hiding this comment.
[❗❗ correctness]
The parameter name change from lockAmount to amount should be verified against the lockAmount method's expected parameters. Ensure that this change does not affect the method's functionality or any dependent code. If lockAmount is a method from an external library or API, double-check the documentation to confirm that amount is the correct parameter name.
| method: (finalOptions.method ?? 'GET'), | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| requestBody: (finalOptions as any).body, |
There was a problem hiding this comment.
[maintainability]
Casting finalOptions to any to access body can lead to runtime errors if finalOptions does not have a body property. Consider using a more specific type or checking if body exists before accessing it.
| responseBody, | ||
| }, | ||
| ); | ||
| // Optional: You could throw a custom error here |
There was a problem hiding this comment.
[design]
Consider throwing a custom error instead of a generic Error. This can provide more context and make error handling more robust and maintainable.
Fix BA consume for v6 api
Add more logging in challenges service (payment generation)