fix(fast-usdc): consider encumberedBalance in withdrawCalc #10870
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
daa6c38
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa645b5c.agoric-sdk.pages.dev |
| Branch Preview URL: | https://dc-fu-wd.agoric-sdk.pages.dev |
dckc
changed the title
dckc
force-pushed
the
dc-fu-wd
branch
2 times, most recently
from
893ff4b to
3c74e53
Compare
| @@ -112,6 +152,8 @@ export const withdrawCalc = (shareWorth, { give, want }) => { | |||
| const { denominator: sharesOutstanding, numerator: poolBalance } = shareWorth; | |||
| !isGTE(want.USDC, poolBalance) || | |||
| Fail`cannot withdraw ${q(want.USDC)}; only ${q(poolBalance)} in pool`; | |||
| isGTE(unencumberedBalance, want.USDC) || | |||
| Fail`cannot withdraw ${q(want.USDC)}; ${q(encumberedBalance)} is in use; stand by for pool to return to ${q(poolBalance)}`; | |||
There was a problem hiding this comment.
I'm not sure what to do about this but I'm pretty sure these Amounts show up as [an Object] or some such thing on the user level, which I assume this messaging is intended for, so maybe render the values instead?
There was a problem hiding this comment.
q(...) renders them. for example:
RemoteError(error:captp:far-zoeTest#20001)#3: cannot withdraw ***"brand":"[Alleged: USDC brand]","value":"[7000000n]"***; ***"brand":"[Alleged: USDC brand]","value":"[5879999n]"*** is in use; stand by for pool to return to ***"brand":"[Alleged: USDC brand]","value":"[10000001n]"***
|
|
||
| // 3. Alice proposes to withdraw 7 USDC | ||
| await t.throwsAsync(E(alice).withdraw(t, 0.7), { | ||
| message: /cannot withdraw .* in use; stand by/, |
There was a problem hiding this comment.
Related to my comment above, possibly make this expected string more explicit
There was a problem hiding this comment.
OK.
I'm generally reluctant to commit to specific error messages in our API, but don't suppose I have a solid justification.
| @@ -72,7 +76,7 @@ test('withdrawal after deposit OK', t => { | |||
| }); | |||
| mustMatch(proposal, shapes.withdraw); | |||
|
|
|||
| const actual = withdrawCalc(state1, proposal); | |||
| const actual = withdrawCalc(state1, proposal, pDep.give); | |||
There was a problem hiding this comment.
I know this already existed but I'm not sure what pDep means... "proposal for deposit"? It's not immediately clear what the connection is between that and the pool allocation. Could we infer it from shareWorth instead?
There was a problem hiding this comment.
It's not immediately clear what the connection is between [proposal for deposit] and the pool allocation.
The connection is that proposal.give is what the deposit handler transfers into the pool:
agoric-sdk/packages/fast-usdc/src/exos/liquidity-pool.js
Lines 266 to 269 in 3c74e53
Could we infer it from
shareWorthinstead?
Not quite; shareWorth.numerator is off by 1E-6 USDC so that it never goes to zero. It's a bit of a design wart, but I haven't figured out a better way.
- advise caller to stand by for pool to be replenished
- pass pool allocation, encumbered balance to withdrawCalc
- push checkPoolBalance down into pool-share-math
- take allocation rather than stateful seat
- push dust calculations down to pool-share-math
closes: #10856
Description
Security Considerations
Prevents contract from becoming unavailable due to an inconsistent state.
Scaling / Documentation Considerations
n/a
Testing Considerations
Upgrade Considerations
not yet deployed