[Clarity] Complete Accepted Withdrawal Request #243

setzeus · 2024-06-19T14:54:43Z

Description

This PR deals with #95, #96, & #97 with the focus of initiating, accepting & rejecting withdrawal requests. As a signer, once completed, I want to mark a withdrawal as completed on-chain & therefore burning the sBTC.

Type of Change

New feature planned in #95 that comes from the sBTC Clarity documentation.

Does this introduce a breaking change?

No.

Are documentation updates required?

No.

Testing information

Six different unit tests included here that can be ran CDing into 'contracts' & running 'pnpm test.'

Checklist

Code is commented where needed
Unit test coverage for new or modified code paths
pnpm test passes
Changelog is updated
Tag 1 of @person1 or @Person2

djordon

This largely looks good, but I think we can simplify the 'complete-many' function by specializing it into two functions or just have it care about acceptances. Also, I don't think we should throw an error if the fee was wrong. Maybe we should print something instead?

contracts/contracts/sbtc-withdrawal.clar

djordon · 2024-06-24T18:26:09Z

contracts/contracts/sbtc-withdrawal.clar

+      ;; Check that fee is not higher than requesters max fee
+      (asserts! (<= fee requested-max-fee) ERR_FEE_TOO_HIGH)


This is an interesting check. It made me think of the withdrawal flow:

User makes a withdrawal request.

The signers decide to act on the request.

The signers submit a BTC transaction fulfilling the request.

The signers wait for the transaction to be confirmed and then make a contract call to this function.

If the signers somehow make a mistake and submit a transaction with a fee that was higher than the users' max fee, what then? With this check the contract call fails but it probably should not fail since the transaction has been confirmed.

Hmm, that's an interesting scenario. I mean, the withdrawal hasn't been fulfilled if the signers exceed the users' max fee. I think the user should be able to reclaim their sBTC, or at least a portion of it, in that scenario.

Hmmm, it's not obvious what the right course of action should be here. Giving them some back feels right but if we do that then the amount minted is no longer 1:1 with the amount under the signers' control. Well, I'm okay with letting this pass for now and we can revisit later. The sBTC is still locked, so I think it's fine.

contracts/contracts/sbtc-withdrawal.clar

djordon · 2024-06-24T18:41:52Z

contracts/contracts/sbtc-withdrawal.clar

+    ;; Call into registry to confirm accepted withdrawal
+    (try! (contract-call? .sbtc-registry complete-withdrawal request-id false none none none none))


Shouldn't this be the following?

(try! (contract-call? .sbtc-registry complete-withdrawal request-id false none (some signer-bitmap) none none))

Also, does it matter?

Good call. I believe the signer-bitmap is needed just for the print event.

I see, so is it actually not needed?

Sorry I was confused and unintentionally confusing before. I think the only thing that needs to change here is changing the none in the third argument in the complete-withdrawal function call to (some signer-bitmap).

djordon · 2024-06-24T18:55:38Z

contracts/contracts/sbtc-withdrawal.clar

+      (fold complete-individual-withdrawal-helper withdrawals (ok u0))
+  )
+)
+
+(define-private (complete-individual-withdrawal-helper (withdrawal {request-id: uint, status: bool, signer-bitmap: uint, bitcoin-txid: (optional (buff 32)), output-index: (optional uint), fee: (optional uint)}) (helper-response (response uint uint)))
+  (match helper-response 
+    index


Why is this short-circuiting? If one of the withdrawals inputs was set erroneously, I wouldn't think that the error would have an effect on any of the other withdrawals in the input.

Hmmm, confused here? If one withdrawal fails they'll all fail. When that happens we want to know where the error is thrown so if the error message is already thrown we don't want to update it but return it.

If one withdrawal fails they'll all fail. When that happens we want to know where the error is thrown so if the error message is already thrown we don't want to update it but return it.

Yup, that makes sense. If we were going to go this route, I think we should should just return a tuple with the error number and the index as distinct fields.

But, I was thinking that the complete-withdrawals function would always try each withdrawal input and either:

Return the result, so return (list 100 (optional uint)) (or something like that) where none means success and (some uint) is the error number; or

Return a tuple for the failures, so return (list 100 {error_number: uint, index: uint}).

The downside is that this would ideally lead to (list 100 none) (or whatever type you go with) being committed on chain, while the upside is that the signers wouldn't need to reprocess failures (since they would be making the same contract call with kinda the same inputs). If we short-circuit then there could be withdrawals after the failed entry that can succeed and the signers need to make another contract call to try those.

Thoughts?

Re: error messages
We can change the return of the (ok response) to include a tuple but not sure I follow how returning a tuple for the error case would work? All error messages across the protocol currently return a uint, including this a similar list mechanic found in sbtc-deposit. I don't think breaking this convention makes sense.

Re: processing w/ failures
This is a great point that I think merits more thought since it'd refactor this mechanic in other places. I'd like to leave this as-is & open a discussion for further consideration, does that work?

Re: error messages We can change the return of the (ok response) to include a tuple but not sure I follow how returning a tuple for the error case would work? All error messages across the protocol currently return a uint, including this a similar list mechanic found in sbtc-deposit. I don't think breaking this convention makes sense.

Eh, I'm more than okay with breaking this convention here (actually, is it only a convention or is it required?). But I think we can side-step this issue and just figure out if we should short-circuit.

Re: processing w/ failures This is a great point that I think merits more thought since it'd refactor this mechanic in other places. I'd like to leave this as-is & open a discussion for further consideration, does that work?

Great idea, let's leave this alone for now until we discuss it with everyone else.

Uint as error messages are a universal convention (& requirement for a lot of trait contracts), not strictly a requirement.

Copy, opened here: #261

djordon · 2024-06-24T19:09:34Z

contracts/contracts/sbtc-withdrawal.clar

+  )
+)
+;; Reject multiple withdrawal requests
+(define-public (complete-withdrawals (withdrawals (list 100 {request-id: uint, status: bool, signer-bitmap: uint, bitcoin-txid: (optional (buff 32)), output-index: (optional uint), fee: (optional uint)})))


I think we can simplify this functionality (or break it up into complete-accept-withdrawals and complete-reject-withdrawals or whatever you want to call it). The signers need to wait for a bitcoin confirmation to complete an accepted withdrawal, but they can complete a rejection right away (well, right after "voting"). So bundling acceptances makes sense, we find out about them in bundles when the transaction package gets confirmed on bitcoin. The flow for rejections should be much faster, since it would happen shortly after it gets confirmed on a stacks Nakamoto block.

Also, I think I prefer breaking them out since it'll be easier to reason about both here and in the signer codebase.

Gotcha. Hmmm, my thinking here is giving the signers flexibility to bundle both accepts & rejects together to submit a single list & therefore make fewer transaction calls. Which, while separating them might improve readability, the real pro-argument here that is signers have to make more transactions, which means they have to pay more fees.

Ahh right, good point.

There is a decent chance that having it as one function will not lead to fewer transaction calls. We might have signers make a contract call for rejects every ~5-30 seconds¹ and then make another for the accepts every ~10 minutes (for new bitcoin blocks) since this approach has the better user experience.

Anyway, I'm okay leaving this as is for now. We can always revisit this later when we get this part nailed down in the signer.

Footnotes

The signers might reject shortly after they find out about it, which is when the request is included in a new Nakamoto block. I'm not sure how quickly Nakamoto blocks are supposed to be created. ↩

contracts/tests/sbtc-withdrawal.test.ts

contracts/contracts/sbtc-withdrawal.clar

djordon

As much as I like the new functionality (which I advocated for) I feel like this approach adds more complexity than it is worth. The issues I have are:

The previous approach of basically returning the index that failed was relatively easy to follow in clarity.
The new approach has a limit of around ~20 errors before the string-to-uint? function fails. This is okay, but adds more complexity that the signers need to handle.

Sorry, but I think I prefer the previous approach. I suppose this also means that there is no need to update the other clarity functions.

djordon · 2024-07-01T15:08:54Z

contracts/contracts/sbtc-withdrawal.clar

+    ;; Call into registry to confirm accepted withdrawal
+    (try! (contract-call? .sbtc-registry complete-withdrawal request-id false none none none none))


I see, so is it actually not needed?

contracts/contracts/sbtc-withdrawal.clar

djordon

Looks good, basically some small nits.

djordon · 2024-07-01T19:04:37Z

contracts/tests/sbtc-withdrawal.test.ts

+    );
+    expect(receipt.value).toEqual(2n);
+  });
+})


nit: new line here

contracts/contracts/sbtc-registry.clar

djordon · 2024-07-01T19:38:29Z

contracts/contracts/sbtc-withdrawal.clar

+    ;; Call into registry to confirm accepted withdrawal
+    (try! (contract-call? .sbtc-registry complete-withdrawal request-id false none none none none))


Sorry I was confused and unintentionally confusing before. I think the only thing that needs to change here is changing the none in the third argument in the complete-withdrawal function call to (some signer-bitmap).

djordon · 2024-07-01T19:49:45Z

contracts/contracts/sbtc-withdrawal.clar

+      ;; Check that fee is not higher than requesters max fee
+      (asserts! (<= fee requested-max-fee) ERR_FEE_TOO_HIGH)


Hmmm, it's not obvious what the right course of action should be here. Giving them some back feels right but if we do that then the amount minted is no longer 1:1 with the amount under the signers' control. Well, I'm okay with letting this pass for now and we can revisit later. The sBTC is still locked, so I think it's fine.

contracts/contracts/sbtc-withdrawal.clar

djordon

Looks good! Feel free to merge once the tests pass.

This reverts commit 556079e.

This reverts commit 3b4a2b1.

setzeus requested review from hstove and radicleart June 19, 2024 14:54

setzeus force-pushed the feat/accept-withdrawal-lock branch 2 times, most recently from 8855a0d to 51e2160 Compare June 23, 2024 18:56

setzeus requested review from djordon and netrome June 24, 2024 14:50

djordon reviewed Jun 24, 2024

View reviewed changes

netrome reviewed Jun 24, 2024

View reviewed changes

contracts/contracts/sbtc-withdrawal.clar Show resolved Hide resolved

setzeus requested a review from FriendsFerdinand June 25, 2024 17:07

setzeus force-pushed the feat/accept-withdrawal-lock branch from d68c040 to 8f983f6 Compare June 25, 2024 17:55

djordon reviewed Jul 1, 2024

View reviewed changes

djordon reviewed Jul 2, 2024

View reviewed changes

contracts/contracts/sbtc-withdrawal.clar Outdated Show resolved Hide resolved

This was referenced Jul 2, 2024

feat: support broadcasting sbtc burn transactions #281

Merged

feat: support withdrawal-reject sbtc transactions #282

Merged

djordon approved these changes Jul 3, 2024

View reviewed changes

setzeus added 14 commits July 3, 2024 10:30

pre-token work

5c0f798

accept withdrawal token actions

3dbaf61

completed design, missing tests

633bed7

tests passing locally

b883071

all tests passing feat/accept-withdrawal-lock

2c3d7c1

feat/accept-withdrawal-lock

94c6b0c

laid down reject-withdrawal

8e1599e

refactored complete-withdrawal parameters

b5f0953

completed multiple withdrawals

11e8c09

optimized new withdrawal helper

3b22846

completed reject withdrawal unit tests

6ab6a73

multiple withdrawals passing

0bea3bb

comment fixes

8ce50b1

updated constant comments

97f469e

setzeus added 7 commits July 3, 2024 10:30

error message refactor

649574d

completed refactor to return more accurate error messages

ed8d6c0

Revert "completed refactor to return more accurate error messages"

9e34ce7

This reverts commit 556079e.

Revert "error message refactor"

f3050d9

This reverts commit 3b4a2b1.

last comments

b5cbdf5

changed name

571bd5d

fixed broken tests

cbf3ff2

setzeus force-pushed the feat/accept-withdrawal-lock branch from 1c4c8b0 to cbf3ff2 Compare July 3, 2024 14:31

setzeus merged commit 467f5a3 into main Jul 3, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clarity] Complete Accepted Withdrawal Request #243

[Clarity] Complete Accepted Withdrawal Request #243

setzeus commented Jun 19, 2024 •

edited

Loading

djordon left a comment

djordon Jun 24, 2024

netrome Jun 24, 2024

djordon Jul 1, 2024

djordon Jun 24, 2024

setzeus Jun 25, 2024 •

edited

Loading

djordon Jul 1, 2024

djordon Jul 1, 2024

djordon Jun 24, 2024 •

edited

Loading

setzeus Jun 25, 2024

djordon Jun 25, 2024 •

edited

Loading

setzeus Jun 25, 2024

djordon Jun 25, 2024

setzeus Jun 26, 2024

djordon Jun 24, 2024

setzeus Jun 25, 2024 •

edited

Loading

djordon Jun 25, 2024

djordon left a comment

djordon Jul 1, 2024

djordon left a comment

djordon Jul 1, 2024

djordon Jul 1, 2024

djordon Jul 1, 2024

djordon left a comment

		;; Check that fee is not higher than requesters max fee
		(asserts! (<= fee requested-max-fee) ERR_FEE_TOO_HIGH)

		;; Call into registry to confirm accepted withdrawal
		(try! (contract-call? .sbtc-registry complete-withdrawal request-id false none none none none))

[Clarity] Complete Accepted Withdrawal Request #243

[Clarity] Complete Accepted Withdrawal Request #243

Conversation

setzeus commented Jun 19, 2024 • edited Loading

Description

Type of Change

Does this introduce a breaking change?

Are documentation updates required?

Testing information

Checklist

djordon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setzeus Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setzeus Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

djordon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon left a comment

Choose a reason for hiding this comment

setzeus commented Jun 19, 2024 •

edited

Loading

setzeus Jun 25, 2024 •

edited

Loading

djordon Jun 24, 2024 •

edited

Loading

djordon Jun 25, 2024 •

edited

Loading

setzeus Jun 25, 2024 •

edited

Loading