Reject unexpected transfers #1241
Conversation
There was a problem hiding this comment.
An alternate approach to structure the MockRuntime could look something like this https://github.com/helix-onchain/builtin-actors/pull/3. It might reduce boilerplate but requires drilling the value sent to the invocation of the call method.
actors/init/tests/init_actor_test.rs
Outdated
| rt.set_value(balance.clone()); | ||
| rt.expect_value(balance.clone()); |
There was a problem hiding this comment.
Perhaps to reduce boilerplate, set_value could internally call expect_value. I can't think of a case where the values ought to be different though doing it implicitly is slightly more magical
There was a problem hiding this comment.
I think the reverse of having expect_value always set the value should work. There appear to be some test cases that use set_value without expect_value, but they're rather suspect so maybe we can remove set_value altogether?
There was a problem hiding this comment.
I believe we also need some storage provider methods to be payable:
- Maybe
SubmitWindowedPoSt? IIRC, storage providers may use this for top-up? But I'm not sure (cc @arajasek). - PreCommitSector/ProveCommitSector/ExtendSectorExpiration(2)/PreCommitSectorBatch(2)/ProveCommitAggregate/ProveREplicaUpdates(2) - To cover deposits.
- DeclareFaults, DeclareFaultsRecovered, RepayDebt - to pay back fee debt.
|
I've marked the methods mentioned from the |
This never a concept before (by default, everything is "payable"), so no. The only reliable way to determine this would be to ask the sentinel team to tell us which methods tend to carry value. Note: this will definitely require a FIP. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1241 +/- ##
==========================================
- Coverage 90.97% 90.87% -0.11%
==========================================
Files 133 133
Lines 26633 26679 +46
==========================================
+ Hits 24230 24245 +15
- Misses 2403 2434 +31
|
I'm not so sure about that (and sorry @alexytsu if it turns out it does, I shouldn't have lined it up for you yet). Perhaps we can open a FIP discussion to determine this though. |
There was a problem hiding this comment.
I think the following should also be payable:
- miner::terminate_sectors (top up after penalties)
- miner::compact_partitions (no reason not to)
- miner::compact_sector_numbers (ditto)
- miner::apply_rewards (necessary, even though miner doesn't inspect the amount)
I could be argued into some more owner/worker-only ones too, though I think they would be quite rare.
actors/init/tests/init_actor_test.rs
Outdated
| rt.set_value(balance.clone()); | ||
| rt.expect_value(balance.clone()); |
There was a problem hiding this comment.
I think the reverse of having expect_value always set the value should work. There appear to be some test cases that use set_value without expect_value, but they're rather suspect so maybe we can remove set_value altogether?
Closes #836
Adds
payableto the runtime which must be called when funds are sent to a method.It aborts the method if funds are received and
payablewas never called.