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

[chore] Reject duplicate deposit requests when first one already accepted | confirmed | failed #1179

Merged
merged 11 commits into from
Jan 9, 2025

Conversation

MCJOHN974
Copy link
Collaborator

@MCJOHN974 MCJOHN974 commented Dec 23, 2024

Description

Closes: #1161

Changes

Now create_deposit requests returns 409 (Conflict) if we already have same deposit accepted

Testing Information

Added integration test for this.

Locally I run it the following way:

 docker compose -f docker/docker-compose.test.yml build   // rebuild emily
 docker compose --file docker/docker-compose.test.yml up.  // start emily
 // open new terminal
 cargo test --package emily-handler --test integration -- deposit::overwrite_deposit --exact --show-output --ignored // run the test

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@MCJOHN974 MCJOHN974 requested review from matteojug and AshtonStephens and removed request for matteojug January 6, 2025 09:59
@MCJOHN974 MCJOHN974 marked this pull request as ready for review January 6, 2025 09:59
@MCJOHN974
Copy link
Collaborator Author

@matteojug is it correct that we want to reject deposits not only if they are Accepted, but Confirmed, Failed and Reprocessing as well?

@MCJOHN974 MCJOHN974 requested a review from matteojug January 6, 2025 14:21
Copy link
Collaborator

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

Welcome to the Emily side of things @MCJOHN974 ! I left a few comments about changes required to handle duplicates deposits in all statuses

emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/src/api/handlers/deposit.rs Outdated Show resolved Hide resolved
emily/handler/tests/integration/deposit.rs Outdated Show resolved Hide resolved
@MCJOHN974 MCJOHN974 changed the title [chore] Reject duplicate deposit requests when first one already accepted [chore] Reject duplicate deposit requests when first one already accepted | confirmed | failed Jan 8, 2025
@MCJOHN974 MCJOHN974 requested review from matteojug and Jiloc January 8, 2025 10:52
@MCJOHN974
Copy link
Collaborator Author

Thanks @matteojug and @Jiloc for your comments, I think last few commits contain your suggestions

emily/handler/tests/integration/deposit.rs Outdated Show resolved Hide resolved
emily/handler/tests/integration/deposit.rs Outdated Show resolved Hide resolved
@MCJOHN974 MCJOHN974 merged commit 042a093 into main Jan 9, 2025
4 checks passed
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.

[Bug]: Return an error for deposit creation in Emily if deposit with the same details already exists
3 participants