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

introduce a crypto::recover_secp_public_key syscall. #707

Merged
merged 5 commits into from
Aug 14, 2022

Conversation

raulk
Copy link
Member

@raulk raulk commented Aug 12, 2022

Supersedes #627 with a clean diff, refresh from master, and some small changes (check commit log).

raulk and others added 2 commits August 12, 2022 19:33
Comment on lines 354 to 362
// prepare actor code
// TODO remove once the EVM smart contract actor is integrated in the builtin-actors bundle
// https://github.com/filecoin-project/ref-fvm/issues/693
self.engine()
.load_code_cid(&state.code, self.blockstore())
.map_err(
|_| syscall_error!(NotFound; "actor code cid does not exist {}", &state.code),
)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracked in #708.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this part of a separate patch?

  • Introduce a secp recover syscall.
  • Code loading?

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2022

Codecov Report

Merging #707 (c7da8eb) into master (f144970) will decrease coverage by 0.25%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #707      +/-   ##
==========================================
- Coverage   50.92%   50.66%   -0.26%     
==========================================
  Files         120      120              
  Lines        9691     9742      +51     
==========================================
+ Hits         4935     4936       +1     
- Misses       4756     4806      +50     
Impacted Files Coverage Δ
fvm/src/gas/price_list.rs 24.48% <0.00%> (-0.74%) ⬇️
fvm/src/kernel/default.rs 15.64% <0.00%> (-0.18%) ⬇️
fvm/src/syscalls/crypto.rs 0.00% <0.00%> (ø)
fvm/src/syscalls/mod.rs 0.00% <0.00%> (ø)
sdk/src/actor.rs 0.00% <ø> (ø)
sdk/src/crypto.rs 0.00% <0.00%> (ø)
shared/src/crypto/signature.rs 0.00% <0.00%> (ø)
ipld/amt/src/node.rs 86.82% <0.00%> (+0.25%) ⬆️

@raulk raulk requested review from Stebalien and mriise August 12, 2022 19:00
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Logic LGTM. I'd do this in two PRs as one is a new syscall, and one supports EVM code loading. But I don't feel too strongly about that.

I'd also change load_code so we don't have to clone the module on every call (we don't use the return value).

Comment on lines 211 to 216
/// Attempts to load Actor Code CID from the blockstore and
/// instantiate a wasmtime Module from the WASM bytecode if the
/// given code CID is present, otherwise returns None.
///
/// Implicitly caches instantiated modules.
pub fn load_code_cid<BS: Blockstore>(
Copy link
Member

Choose a reason for hiding this comment

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

install_code? We don't actually need to return it.

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM. I'll move this part to a different patch and we can address there.

Copy link
Contributor

@mriise mriise left a comment

Choose a reason for hiding this comment

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

LGTM, +1 on splitting this PR though (not the end of the world if it isnt though).

@raulk
Copy link
Member Author

raulk commented Aug 14, 2022

The extra feature has been moved to #711 and this PR is now strictly about the recover_secp_public_key syscall.

@raulk raulk merged commit 7c1e9ab into master Aug 14, 2022
@raulk raulk deleted the raulk/recover_secp_public_key branch August 14, 2022 22:50
Stebalien pushed a commit that referenced this pull request Aug 29, 2022
Co-authored-by: karim <karim.dev@gmail.com>
shamb0 pushed a commit to shamb0/ref-fvm that referenced this pull request Sep 23, 2022
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.

4 participants