Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to FVM SDK 3.0.0-alpha.22, shared 3.0.0-alpha.17, ipld_encoding 0.3.3 #1095

Merged
merged 3 commits into from
Jan 25, 2023

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Jan 24, 2023

This is a pre-factor that updates this repo to rely on FVM3 deps, without introducing or supporting any FEVM functionality. The new crates themselves do include some FEVM changes:

  • hyperspace support (I'm hoping this is okay, it's just a testnet)
  • The f4 address scheme, which is in FIP-0048, which has been Accepted as of today

Most of the changes here result from integrating the latest changes in the https://github.com/helix-onchain/filecoin repo.

Needs:

self.rt.message().receiver().id().unwrap()
}

// TODO: What's the best way to avoid this duplication?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help sought here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution now looks good, please remove this comment.

&& expected_msg.params == params
&& expected_msg.value == value,
);
assert_eq!(expected_msg.to, *to);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes debugging failures easier.

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2023

Codecov Report

Merging #1095 (34b2534) into master (f1d0fb0) will decrease coverage by 0.18%.
The diff coverage is 58.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1095      +/-   ##
==========================================
- Coverage   89.34%   89.17%   -0.18%     
==========================================
  Files          93       93              
  Lines       19613    19637      +24     
==========================================
- Hits        17524    17511      -13     
- Misses       2089     2126      +37     
Impacted Files Coverage Δ
runtime/src/actor_error.rs 77.41% <ø> (ø)
test_vm/src/lib.rs 88.31% <0.00%> (-1.52%) ⬇️
runtime/src/test_utils.rs 82.58% <36.36%> (-0.72%) ⬇️
actors/datacap/src/lib.rs 79.29% <80.95%> (+1.73%) ⬆️
actors/miner/src/state.rs 80.88% <100.00%> (ø)
actors/miner/src/types.rs 95.83% <100.00%> (ø)
actors/power/src/types.rs 100.00% <100.00%> (ø)
actors/miner/src/deadline_info.rs 79.77% <0.00%> (-16.86%) ⬇️
actors/power/src/lib.rs 84.66% <0.00%> (-0.22%) ⬇️
actors/miner/src/lib.rs 82.82% <0.00%> (-0.14%) ⬇️
... and 4 more


fn set_state_root(&mut self, root: &Cid) -> Result<(), ActorError> {
Ok(fvm::sself::set_root(root)?)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok if we're going to need these for the EVM actor, otherwise would prefer not to add them.

@anorth
Copy link
Member

anorth commented Jan 24, 2023

Looks good after we resolve the token library syscall thing

let msg = Messenger { rt };
let intermediate = hook.call(&&msg).actor_result()?;
let msg = SyscallProvider { rt };
let intermediate = hook.call(as_token(&mut st, &msg).runtime()).actor_result()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is even more funky now as there's no logical need for the state to be involved in getting a runtime abstraction. You actually just need an ActorRuntime, as constructed in as_token. Copy just that bit of construction here?

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can clean up the hook call, otherwise this looks good (after deps land).

self.rt.message().receiver().id().unwrap()
}

// TODO: What's the best way to avoid this duplication?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution now looks good, please remove this comment.

@arajasek arajasek merged commit 60b992a into master Jan 25, 2023
@arajasek arajasek deleted the asr/update-fvm branch January 25, 2023 16:59
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
…g 0.3.3 (filecoin-project#1095)

* Update to FVM SDK 3.0.0-alpha.22, shared 3.0.0-alpha.17, ipld_encoding 0.3.3

* Datacap: Turn Messenger struct into SyscallProvider

* Update frc42_dispatch, frc46_tokn, fvm_actor_utils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants