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

Fix: Refund failed FT deposits #253

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

jbencin
Copy link
Member

@jbencin jbencin commented Apr 10, 2023

Description

If a fungible token deposit onto a subnet fails, create a withdrawal transaction to allow the user to withdraw it from the contract

Applicable issues

Additional info (benefits, drawbacks, caveats)

There is no attempt here to handle the case of a partial success (for example, a user deposits 50 tokens and only 20 appear on the subnet). This shouldn't happen, and if it does I would think it indicates a problem with the Clarity contract.

Should I move make_withdrawal_event() out of blocks.rs and into clarity_vm/withrdawal.rs?

Also including a couple minor fixes, see commit messages for details.

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Merging #253 (8369a9e) into master (a762f8f) will not change coverage.
The diff coverage is n/a.

❗ Current head 8369a9e differs from pull request most recent head b32b7eb. Consider uploading reports for the commit b32b7eb to get more accurate results

@@           Coverage Diff           @@
##           master     #253   +/-   ##
=======================================
  Coverage   93.47%   93.47%           
=======================================
  Files           6        6           
  Lines         337      337           
=======================================
  Hits          315      315           
  Misses         22       22           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jbencin jbencin force-pushed the fix/handle-failed-ft-deposits branch from ded7eeb to 202e580 Compare April 11, 2023 13:56
@jbencin jbencin requested a review from obycode April 11, 2023 18:30
Copy link
Member

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Just a few minor comments. Looks great!

jbencin added a commit to jbencin/stacks-subnets that referenced this pull request Apr 13, 2023
@jbencin
Copy link
Member Author

jbencin commented Apr 13, 2023

Just made all the changes requested, except removing the TODO, which I'll do as part of #256

@jbencin jbencin force-pushed the fix/handle-failed-ft-deposits branch from 8369a9e to b32b7eb Compare April 13, 2023 18:45
Copy link
Member

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jbencin!

@jbencin jbencin merged commit 84fb7e0 into hirosystems:master Apr 13, 2023
@jbencin jbencin deleted the fix/handle-failed-ft-deposits branch April 18, 2023 19:13
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.

Handle failed deposits gracefully
2 participants