Skip to content
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

[Job Launcher Server] Move withdrawal creation logic to payments module #3147

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

flopez7
Copy link
Contributor

@flopez7 flopez7 commented Mar 3, 2025

Issue tracking

#3137

Context behind the change

Move the withdrawal creation logic to the payments module

How has this been tested?

Deployed locally and launched one new escrow

Release plan

None

Potential risks; What to monitor; Rollback plan

Check payment creation in DB

Copy link

vercel bot commented Mar 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
human-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 6, 2025 9:24am
human-dashboard-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 6, 2025 9:24am
staking-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 6, 2025 9:24am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
faucet-frontend ⬜️ Ignored (Inspect) Visit Preview Mar 6, 2025 9:24am
faucet-server ⬜️ Ignored (Inspect) Visit Preview Mar 6, 2025 9:24am

Copy link
Collaborator

@portuu3 portuu3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think withdrawal function should include the balance check so that is not possible to withdraw if the balance is not enough

@flopez7
Copy link
Contributor Author

flopez7 commented Mar 4, 2025

I think withdrawal function should include the balance check so that is not possible to withdraw if the balance is not enough

There's a problem moving it directly to the method, as the check would be executed at the end, so if the user has no balance, the job entity would have already been created.
There are many options here:

  • Keep it separate, that way we can do the check at the beginning without duplicating it.
  • If you want to add it inside the method, you could run the creation of the payment at the beginning, making the check.

IMO, we should keep them separately. This ensures that the balance is verified before any other operations are performed, preventing the creation of unnecessary entities and maintaining data consistency. Also, the createWithdrawalPayment method will remain focused on creating the withdrawal payment

cc: @portuu3 @Dzeranov

@portuu3
Copy link
Collaborator

portuu3 commented Mar 5, 2025

I think withdrawal function should include the balance check so that is not possible to withdraw if the balance is not enough

There's a problem moving it directly to the method, as the check would be executed at the end, so if the user has no balance, the job entity would have already been created. There are many options here:

  • Keep it separate, that way we can do the check at the beginning without duplicating it.
  • If you want to add it inside the method, you could run the creation of the payment at the beginning, making the check.

IMO, we should keep them separately. This ensures that the balance is verified before any other operations are performed, preventing the creation of unnecessary entities and maintaining data consistency. Also, the createWithdrawalPayment method will remain focused on creating the withdrawal payment

cc: @portuu3 @Dzeranov

IMO, withdraw function should for sure include a balance check.
About the order of payment entity and job entity creation, actually I think the payment should be created before the job entity, this way we avoid an edge case where we create the job and don't charge the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants