Fix calibnet wallet test#7183
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/message_pool/msgpool/provider.rs`:
- Around line 94-100: The get_actor_after method has a contract violation in the
None branch where there is no child tipset. When load_child_tipset returns None
(at the head), the code incorrectly uses ts.parent_state() which returns the
state BEFORE ts executes, but the method contract requires the state AFTER ts
executes. Replace the None branch to compute the true post-tipset state by
applying the tipset's state transitions instead of using the parent state,
ensuring that stale pre-tipset actor state is not fed into message pool
validation paths.
In `@src/message_pool/msgpool/test_provider.rs`:
- Around line 155-178: The get_actor_before method always returns
Ok(Some(actor)) with a default balance when an actor is not found in the test
data, but according to the Provider contract it should return Ok(None) when the
actor is absent. Modify the method to check if the actor exists in
inner.balances; if the get call returns None, return Ok(None) immediately
instead of proceeding to create an ActorState with default values. Only
construct and return the ActorState when the actor actually exists in the test
data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d379f2a-3f89-441e-bdb0-809467f2ea14
📒 Files selected for processing (4)
src/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/test_provider.rstests/common/calibnet_wallet_helpers.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
| fn get_actor_after(&self, addr: &Address, ts: &Tipset) -> Result<ActorState, Error> { | ||
| let state_root = match self.load_child_tipset(ts)? { | ||
| Some(child) => *child.parent_state(), | ||
| None => *ts.parent_state(), | ||
| }; | ||
| let state = StateTree::new_from_root(self.db(), &state_root) | ||
| .map_err(|e| Error::Other(e.to_string()))?; |
There was a problem hiding this comment.
get_actor_after can return pre-tipset state on head tipsets.
On Line 97, the None branch uses *ts.parent_state(), which is the state before ts executes, while the contract on Line 39 says this method is after ts’s messages. When ts has no child yet (common at head), this can feed stale actor sequence/balance into message-pool validation paths.
Please switch the no-child branch to a true post-ts state computation path (Lotus-equivalent TipSetState(ts) semantics), instead of falling back to ts.parent_state().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/message_pool/msgpool/provider.rs` around lines 94 - 100, The
get_actor_after method has a contract violation in the None branch where there
is no child tipset. When load_child_tipset returns None (at the head), the code
incorrectly uses ts.parent_state() which returns the state BEFORE ts executes,
but the method contract requires the state AFTER ts executes. Replace the None
branch to compute the true post-tipset state by applying the tipset's state
transitions instead of using the parent state, ensuring that stale pre-tipset
actor state is not fed into message pool validation paths.
Source: Linked repositories
| fn get_actor_before(&self, addr: &Address, _ts: &Tipset) -> Result<Option<ActorState>, Error> { | ||
| let inner = self.inner.lock(); | ||
| let canonical = inner.resolve_addr(addr); | ||
| let balance = match inner.balances.get(&canonical) { | ||
| Some(b) => b.clone(), | ||
| None => TokenAmount::from_atto(10_000_000_000_u64), | ||
| }; | ||
| let sequence = inner | ||
| .state_sequence | ||
| .get(&canonical) | ||
| .copied() | ||
| .unwrap_or_default(); | ||
| let actor = ActorState::new( | ||
| // Account Actor code (v10, calibnet) | ||
| Cid::try_from("bafk2bzacebhfuz3sv7duvk653544xsxhdn4lsmy7ol7k6gdgancyctvmd7lnq") | ||
| .unwrap(), | ||
| Cid::default(), | ||
| balance, | ||
| sequence, | ||
| None, | ||
| ); | ||
|
|
||
| Ok(Some(actor)) | ||
| } |
There was a problem hiding this comment.
TestApi::get_actor_before never returns None, so fallback behavior is untestable.
Line 177 always returns Ok(Some(actor)) (with defaulted state), even when actor state is absent. That diverges from the Provider contract (“None if absent”) and prevents get_state_sequence’s get_actor_after fallback path from being exercised in tests.
🔧 Suggested fix
fn get_actor_before(&self, addr: &Address, _ts: &Tipset) -> Result<Option<ActorState>, Error> {
let inner = self.inner.lock();
let canonical = inner.resolve_addr(addr);
+ let has_actor =
+ inner.state_sequence.contains_key(&canonical) || inner.balances.contains_key(&canonical);
+ if !has_actor {
+ return Ok(None);
+ }
+
let balance = match inner.balances.get(&canonical) {
Some(b) => b.clone(),
None => TokenAmount::from_atto(10_000_000_000_u64),
};
let sequence = inner
.state_sequence
.get(&canonical)
.copied()
.unwrap_or_default();
@@
- Ok(Some(actor))
+ Ok(Some(actor))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn get_actor_before(&self, addr: &Address, _ts: &Tipset) -> Result<Option<ActorState>, Error> { | |
| let inner = self.inner.lock(); | |
| let canonical = inner.resolve_addr(addr); | |
| let balance = match inner.balances.get(&canonical) { | |
| Some(b) => b.clone(), | |
| None => TokenAmount::from_atto(10_000_000_000_u64), | |
| }; | |
| let sequence = inner | |
| .state_sequence | |
| .get(&canonical) | |
| .copied() | |
| .unwrap_or_default(); | |
| let actor = ActorState::new( | |
| // Account Actor code (v10, calibnet) | |
| Cid::try_from("bafk2bzacebhfuz3sv7duvk653544xsxhdn4lsmy7ol7k6gdgancyctvmd7lnq") | |
| .unwrap(), | |
| Cid::default(), | |
| balance, | |
| sequence, | |
| None, | |
| ); | |
| Ok(Some(actor)) | |
| } | |
| fn get_actor_before(&self, addr: &Address, _ts: &Tipset) -> Result<Option<ActorState>, Error> { | |
| let inner = self.inner.lock(); | |
| let canonical = inner.resolve_addr(addr); | |
| let has_actor = | |
| inner.state_sequence.contains_key(&canonical) || inner.balances.contains_key(&canonical); | |
| if !has_actor { | |
| return Ok(None); | |
| } | |
| let balance = match inner.balances.get(&canonical) { | |
| Some(b) => b.clone(), | |
| None => TokenAmount::from_atto(10_000_000_000_u64), | |
| }; | |
| let sequence = inner | |
| .state_sequence | |
| .get(&canonical) | |
| .copied() | |
| .unwrap_or_default(); | |
| let actor = ActorState::new( | |
| // Account Actor code (v10, calibnet) | |
| Cid::try_from("bafk2bzacebhfuz3sv7duvk653544xsxhdn4lsmy7ol7k6gdgancyctvmd7lnq") | |
| .unwrap(), | |
| Cid::default(), | |
| balance, | |
| sequence, | |
| None, | |
| ); | |
| Ok(Some(actor)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/message_pool/msgpool/test_provider.rs` around lines 155 - 178, The
get_actor_before method always returns Ok(Some(actor)) with a default balance
when an actor is not found in the test data, but according to the Provider
contract it should return Ok(None) when the actor is absent. Modify the method
to check if the actor exists in inner.balances; if the get call returns None,
return Ok(None) immediately instead of proceeding to create an ActorState with
default values. Only construct and return the ActorState when the actor actually
exists in the test data.
1 participant
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests