feat: add storage commitments to blobs #304
Conversation
ukstv
changed the title
|
@avichalp @carsonfarmer WIP review would be helpful. Although, if you are busy, feel free to skip. |
carsonfarmer
left a comment
carsonfarmer
left a comment
There was a problem hiding this comment.
Very nice! Love the code cleanups for sure. This all looks logically sound. I've made some naming suggestions, and a comment about naming versus expected behavior. Otherwise, I think this is directionally correct.
| GetPendingBlobs = frc42_dispatch::method_hash!("GetPendingBlobs"), | ||
| FinalizeBlob = frc42_dispatch::method_hash!("FinalizeBlob"), | ||
| DeleteBlob = frc42_dispatch::method_hash!("DeleteBlob"), | ||
| GetStorageStaked = frc42_dispatch::method_hash!("GetStorageStaked"), |
There was a problem hiding this comment.
I don't have a strong opinion here, but I think in our earlier discussions, we were going to call this "commitments" versus "staking" which technically happens elsewhere? So that would make this one (for example) GetStorageCommitment..?
There was a problem hiding this comment.
Fair enough. Changed it to "commitment" and friends for uniformity.
| /// Params to decrease storage staked per validator. | ||
| #[derive(Clone, Debug, Serialize_tuple, Deserialize_tuple)] | ||
| pub struct UnstakeStorageParams { | ||
| pub address: Address, |
There was a problem hiding this comment.
What do we need the address property for here? If we're always going to assert_message_source (which of course, we should!), could we just pull this from the origin or caller?
There was a problem hiding this comment.
That's fair concern. It is not clear who the origin or the caller can be though considering the call could be originated from an ETH contract. It is similar to how credits work IMO:
- when buying a credit, neither origin nor caller is used as the recipient, it is an explicitly called receiver: https://github.com/hokunet/ipc/blob/ec489d54c028f64942541646bb0483831c675a1b/fendermint/actors/blobs/src/actor.rs#L74
- when approving credit allowance, same: https://github.com/hokunet/ipc/blob/ec489d54c028f64942541646bb0483831c675a1b/fendermint/actors/blobs/src/actor.rs#L87
- when revoking credit allowance, same: https://github.com/hokunet/ipc/blob/ec489d54c028f64942541646bb0483831c675a1b/fendermint/actors/blobs/src/actor.rs#L111
- I'd say the same trick should be done when adding a blob.
| address: id_addr, | ||
| storage: 200, | ||
| }; | ||
| let result = rt.call::<BlobsActor>( |
There was a problem hiding this comment.
Should this error? Is it (un)expected that you can unstake more than you have staked? It seems like something would have to have gone wrong to try to unstake more than you have staked?
One more (open) question: Should you be able to unstake to zero? Or should we have an explicit unbond method? In that case, should unstake prevent you from unstaking beyond some minimum threshold?
There was a problem hiding this comment.
I fail to see a big enough difference between single "uncommit to zero" and "uncommit to the minimum and then unbond" today, sorry. I mean, why we should do either? What's our goal? What are we trying to optimise?
For now storage commitment is a sort of second citizen of a system. Making it the first citizen now would add more constraints to the node operators, which AFAIK is quite a big issue still. IMHO, it makes more sense to start with a simple approach first and then add complications (as in watches! non-derogatory).
I definitely see the reason why we might want to separate uncommit and unbond in time. That would introduce some more friction with regards to validators money, which supposedly is net positive for the economy. But, again, that is a complication we can live without for some time.
I'd say, we should tie together the minimum storage amount, unbonding period, and probably something else - we probably want to do start data flowing on changed storage, and when that model is ready, go implement it.
Just in case, I consider this indeed as an open question worth modelling. Have stuff like that tracked in Notion: https://www.notion.so/Memo-Storage-commitment-operationalized-12fdfc9427de807c8eeacbd1dbf2d0f6?pvs=4
| } | ||
|
|
||
| pub fn stake_storage(&mut self, validator: Address, amount: u64) -> Result<StorageStakedReturn, ActorError> { | ||
| let storage_commitment = self.capacity_commited.entry(validator).and_modify(|v| *v += amount).or_insert(amount); |
There was a problem hiding this comment.
If the default behavior is to add amount (versus set), then we probably need to rename this method to something like add_storage_stake, and remove_storage_stake, otherwise, my assumption would be that a call to stake_storage would set or update my stake to amount. FWIW I think your the behavior here is good, we should just update the method name to reflect the behavior.
fendermint/actors/blobs/src/state.rs
Outdated
| }) | ||
| } | ||
|
|
||
| pub fn unstake_storage(&mut self, validator: Address, amount: u64) -> anyhow::Result<StorageStakedReturn, ActorError> { |
For now it only exposes functions, and modifies the state. No token handling.
Got rid of some code duplication.