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

feat(withdrawal): implement fa withdrawal #560

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

zcabter
Copy link
Collaborator

@zcabter zcabter commented Sep 12, 2024

Context

Adds support for direct fa withdraw from external operation RunFunction and through smart function call.

Because this is implemented as smart function call, also closes https://linear.app/tezos/issue/JSTZ-112/fa-withdrawal-smart-function-call for free.

Description

This design is based on TZIP-029 except that the user initiates the withdrawal by calling the proxy smart function instead of the protocol.

This design has a few benefits over TZIP-029

  1. Simpler and symmetric UX - Just like how users don't need to know the ticket details of a deposit, users should not need to know ticket details to withdraw. This information is correctly exposed in proxy smart function only.
  2. Simplifies implementation in the protocol
  3. Removes 1 level of indirection. It is obvious now that the proxy smart function needs to implement a withdraw bridge to withdraw their tokens

Withdraw and FA withdraw share similarities in how they are validated. The only difference between their request objects is the body, so the validate_withdraw_request was refactored with a generic <T: Deserialize>.

Doc string comments were added to TicketTable::add/sub as the return type is an ambiguous Amount

Manually testing the PR

cargo test -p jstz_proto fa_withdraw

@zcabter zcabter force-pushed the ryan-jstz-29/fa-withdrawal branch from c558c63 to d6b5af8 Compare September 12, 2024 14:38
@zcabter zcabter requested a review from johnyob September 12, 2024 14:38
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 93.17739% with 35 lines in your changes missing coverage. Please review.

Project coverage is 36.24%. Comparing base (b6cc1bf) to head (cc627c8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/jstz_proto/src/executor/fa_withdraw.rs 93.02% 1 Missing and 14 partials ⚠️
crates/jstz_proto/src/error.rs 0.00% 13 Missing ⚠️
crates/jstz_proto/src/executor/smart_function.rs 97.67% 0 Missing and 3 partials ⚠️
crates/jstz_proto/src/executor/withdraw.rs 78.57% 0 Missing and 3 partials ⚠️
crates/jstz_core/src/kv/outbox.rs 95.00% 0 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
crates/jstz_mock/src/lib.rs 100.00% <100.00%> (ø)
crates/jstz_proto/src/api/smart_function.rs 78.80% <100.00%> (+8.83%) ⬆️
crates/jstz_proto/src/context/ticket_table.rs 93.20% <ø> (+0.61%) ⬆️
crates/jstz_proto/src/executor/mod.rs 23.25% <ø> (ø)
crates/jstz_proto/src/receipt.rs 54.54% <ø> (ø)
crates/jstz_core/src/kv/outbox.rs 89.21% <95.00%> (+0.18%) ⬆️
crates/jstz_proto/src/executor/smart_function.rs 85.47% <97.67%> (+2.33%) ⬆️
crates/jstz_proto/src/executor/withdraw.rs 91.83% <78.57%> (-2.40%) ⬇️
crates/jstz_proto/src/error.rs 21.12% <0.00%> (+0.43%) ⬆️
crates/jstz_proto/src/executor/fa_withdraw.rs 93.02% <93.02%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6cc1bf...cc627c8. Read the comment docs.

Copy link
Collaborator

@johnyob johnyob left a comment

Choose a reason for hiding this comment

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

Overall lgtm 🚀

Small nit on Rust style

})
}

/// Execute the [FaWithdrawal] request atomically. See [fa_withdraw].
Copy link
Collaborator

Choose a reason for hiding this comment

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

stylistic nit: But wouldn't it make more sense to have this in an impl for FaWithdraw
e.g. FaWithdraw::execute(rt, tx, source, gas_limit)

@zcabter zcabter force-pushed the ryan-jstz-29/fa-withdrawal branch from d6b5af8 to bb0d0c8 Compare September 18, 2024 12:52
@zcabter zcabter force-pushed the ryan-jstz-29/fa-withdrawal branch from bb0d0c8 to cc627c8 Compare September 18, 2024 15:40
@zcabter zcabter merged commit a211452 into main Sep 18, 2024
5 checks passed
@zcabter zcabter deleted the ryan-jstz-29/fa-withdrawal branch September 18, 2024 17:19
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