fix: misc bugs found by AI - #1733
Conversation
|
Here the document suggests
But the code actually doesn't allow the balance to fall below the sum of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1733 +/- ##
=======================================
Coverage 90.52% 90.52%
=======================================
Files 139 139
Lines 27693 27695 +2
=======================================
+ Hits 25069 25071 +2
Misses 2624 2624
🚀 New features to boost your workflow:
|
|
While validating power state validate_power_state here we are missing upper bound check on the Let me know if this is a real concern i can create a separate issue or change it in this PR. |
|
in amout_vested
could give us the |
|
AddPairKey encoding is not implemented, we are not using it anywhere but still it panics. |
| set.delete(&value)?; | ||
| let new_root = set.flush()?; | ||
| self.outer.set(key, new_root)?; | ||
| if set.collect_keys()?.is_empty() { |
There was a problem hiding this comment.
only need to check this if set.delete returns true
| set.delete(&value)?; | ||
| let new_root = set.flush()?; | ||
| self.outer.set(key, new_root)?; | ||
| if set.collect_keys()?.is_empty() { |
There was a problem hiding this comment.
This is potentially an expensive call to figure out if it's empty, using collect_keys() is not ideal.
So it looks like remove() was added in the original ChainSafe actors code, specs-actors doesn't have a remove() and we don't use a singular remove() anywhere I can find in the actors, there's only a removeAll() use (same in specs-actors). So this was probably added for the purpose of writing some test code.
If we were using it in production paths, it would just result in a bloated HAMT that had leaves that go nowhere (flush() on an empty HAMT yields the empty HAMT CID), so it's messy, but as long as all nodes are doing the same the cost is just in slight state bloat. Fixing this is nice, but since we don't seem to be using it on-chain anywhere it's low risk and probably won't have any impact unless we start using it.
So, to do this properly, Hamt::is_empty() already exists, we should just expose that on Set (runtime/src/util/set.rs):
pub fn is_empty(&self) -> bool {
self.0.is_empty()
}Then you can avoid collect_keys() here.
4 participants
While taking the code from the builtin actor into the
fil-actor-statefor the V18 actor update, coderabbit AI suggested some fixes/comments here.I have fixed couple of them rest were about adding the checks for negative and zero but I verified most of them didn't find anything worth changing but will post in the comments if anybody thinks they are worth it. Also It turns out the changes were not that big.