Delegation creation and spending #894
Conversation
azarovh
requested review from
TheQuantumPhysicist,
erubboli,
iljakuklic,
muursh and
stanislav-tkach
as code owners
azarovh
force-pushed
the
feat/delegate_staking
branch
from
b9c7ee5 to
ac37486
Compare
iljakuklic
left a comment
iljakuklic
left a comment
There was a problem hiding this comment.
Looking forward for the simplifying follow up. Some initial comments below.
| use crypto::random::{CryptoRng, Rng, RngCore}; | ||
| use rstest::rstest; | ||
| use test_utils::random::{make_seedable_rng, Seed}; |
There was a problem hiding this comment.
Unrelated to this PR: It may make sense for test_utils::random to reexport stuff from crypto::random since these are so frequently used together. Also possibly test_utils could reexport rstest to serve as a thin abstraction layer over it. Thoughts?
There was a problem hiding this comment.
I don't mind reexporting crypto::random through test_utils::random.
chainstate/tx-verifier/src/transaction_verifier/input_output_policy.rs
Outdated
Show resolved
Hide resolved
| Ok(()) | ||
| } | ||
|
|
||
| // single DelegateStaking input; zero or one DelegateStakingOutput + any number of SpendShareFromDelegation |
There was a problem hiding this comment.
Why do we allow zero? That basically burns the bridge, unless the balance there is zero.
There was a problem hiding this comment.
... balance is zero + pool is decommissioned.
There was a problem hiding this comment.
this allows tx like DelegateStaking -> LockThenTransfer to completely spend the share without change
There was a problem hiding this comment.
What happens if people attempt to burn the bridge. Meaning, they attempt to spend without the DelegateStaking output while they still have balance?
|
Looking fine. Though I'd appreciate more documentation into this. Also, perhaps combining this with the upcoming changes will make this easier for the eyes. I don't know which should come first. |
I created a PR #902, you can take a look and if you like the approach in general I'll merge it and we can continue combined review here |
Very well. I'll review that, then we merge it into this if it looks good. Cheers! |
azarovh
force-pushed
the
feat/delegate_staking
branch
2 times, most recently
from
10a1bc0 to
8b49635
Compare
There was a problem hiding this comment.
Do we want to put block reward limitations on ConsensusData::None?
There was a problem hiding this comment.
I'd say yes, but the maturity distance is 0
chainstate/tx-verifier/src/transaction_verifier/input_output_policy.rs
Outdated
Show resolved
Hide resolved
azarovh
force-pushed
the
feat/delegate_staking
branch
from
70d114c to
38aabce
Compare
azarovh
force-pushed
the
feat/delegate_staking
branch
from
38aabce to
8db32f0
Compare
This PR introduces 3 new TxOutput types:
CreateDelegationId,DelegateStakingandSpendShareFromDelegation.Here I followed the same design approach as with create/decommission pool but I think we should reconsider it: #893
Some implementation details to pay attention to:
SpendShareFromDelegationdoesn't perform any accounting operation as actual balance decrease is done when spendingDelegateStakingutxoSpendShareFromDelegationis timelocked by the value from the chain configDelegateStackingoutput can only be spent with the key provided byCreateDelegationId::spend_destinationDelegateStackingcan be spent partially by producing anotherDelegateStackingfor a changeCloses #873