External staking txs #54
Conversation
There was a problem hiding this comment.
Okay, my first thought was this was overly complex.
And I saw various errors.
The whole approach is about deeply thinking every code path and ensuring it handles any in-flight transactions using domain knowledge.
This is not safe or scalable. We need to asset a few simple invariants that will detect any potential conflicts and just be responsible to mark where conflicts might occur (much easier than checking every permutation for conflict).
Applying a lock on that user on vault (no deposit, withdraw, or stake) until the pending stake finishes is very safe. And it only provides mild inconvenience to that one user. I would start there, but leave an API that can expand later.
I think something like ValueRange would allow "partial locks" but add a lot more complexity to surrounding computations. Notably, it would be very obvious with it's type, causing exisitng logic to fail to compile, and force updating to handle "ranges of values".
By leaving the same structs and storing something "next to them", which is essential for their validity, it is very easy to overlook it one place and violate constraints. This is the same reason Rust uses Mutex<T> forcing you to take the mutex to touch the type, rather than the C/Java use of a mutex next to an object, which might be forgotten in some code, violating consistency.
Sorry for the long rambling, but this attempt only proved to me why using those locking primitives is the only safe way to start with this. (And we need very clear theoretical arguments why something would always hold if we add a new strategy)
| .users | ||
| .may_load(ctx.deps.storage, &ctx.info.sender)? | ||
| .unwrap_or_default(); | ||
| user.max_lien = user.max_lien.max(lien.amount); |
There was a problem hiding this comment.
Even if lein was properly updated, this doesn't handle multiple in-flight tx on the same lienholder.
eg. I have 40 collateral. I had a lien of 20 on X (only one, my max)
A. Try to stake 10 to X -> max(20, 20 + 10) = 30 .... this passes check, but not persisted.
B. Try to stake 20 to X -> max(20, 20 + 20) = 40 ... this passes check but not persisted.
If both get success acks, then we have 20 + 10 + 20 = 50 bonded.
There was a problem hiding this comment.
Again, you're right. This would require to look for the max of the liens over the sum of the pending ones. Will fix for referenceFixed?
I agree this is overly complex to be able to reason about it clearly, or in a simple way. But, I'm still not convinced the "value ranges" / "partial locking" approach will make our lives simpler.
|
|
||
| let amount = amount.amount; | ||
| // Tx starts here | ||
| // Verify that the user has enough collateral to stake this and the currently pending txs |
There was a problem hiding this comment.
This looks correct, but way too complex not to assume someone will miss a case.
|
This is replaced by #57 so do you want to close it? (I'm try to clean up... there were too many open PRs) |
|
Closing in favour of #57. |
Proof of concept / exploration of transactional remote staking functionality.
Draws from some of the ideas discussed in #49, but does a bare-bones / straightforward impl for clarity. Value ranges are implemented through filtering over transaction types, and a map of pending txs is used for persistence / tracking.
If this is accepted, it can eventually mature and turn into a package or module for implementing the same in other contracts, and supporting other transaction types.
Also, a full impl of this would perhaps require something like two phase commits, as these transactions can involve multiple contracts, and things can fail in any of them.
TODO: