-
Notifications
You must be signed in to change notification settings - Fork 16
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: sbtc limits handling in signer + emily integration #811
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few sanity checks. My main question is why this is handled in a separate sidecar that updates every 5 minutes, rather than during the creation and processing of new transactions. That said, the functionality remains unchanged, so I’m completely okay with it. The only potential edge case I see is if we lowered the cap while signers were validating new transactions, but that’s highly unlikely, and we’re not planning anything like that. Overall, I’m fine with the current implementation!
So my thinking was largely that this value should almost never change and to avoid extra calls to Emily that maybe this would be once an hour or something instead of every block (or 5m as it's coded right now). I also wanted to avoid piling additional things on-top of the block-observer checks in general. I don't have a strong opinion either way though tbh. |
That makes sense. The more I think about it, the only way to make this check synchronous with the deposit request would be to have it act an emergency stop mechanism to halt deposits immediately if something unusual happens. However, this isn’t meant to serve that purpose, so I agree that we’re fine as it is. Keeping the block-observer "lean" is definitely a good goal. |
Note that this PR is still missing tests, hence the draft status. I'm going to work on those next. |
* cap deposits when constructing new transactions * move cap control in the validate method. Sync check for caps * add total amount check * clean up * add tests * fix error * address review comments * centralize limits update in block observer * fix error message Co-authored-by: Matteo Almanza <[email protected]> --------- Co-authored-by: Matteo Almanza <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly nits, LGTM
Description
Addresses sBTC caps described in #854 & #856.
Changes
Other Info
PR #904 added support for getting the current circulating sBTC amount from the
sbtc-token
contract, however I'm not entirely sure how we want to implement total-peg caps.For instance, if the cap is 100 and total-circulating is currently 95, if we get 7 deposit requests of 1 BTC each then that would theoretically put us at 102. Putting the total cap filter on the decision level would mean we only accept 5 of those, but we don't know if those will manage to be signed and swept at this point (another signer might say "no").
What might feel more correct is having the transaction coordinator's packaging only construct packages that keep us under the total cap, and then have each signer validate that against the total-circulating amount that their own Stacks node reports during the signing-validation process.