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

feat(cheatcodes): base implementation of cheatcode system #58

Merged
merged 48 commits into from
Apr 24, 2023

Conversation

tchataigner
Copy link
Contributor

Description

This PR introduces cheatcodes in Kythera. The inspiration for what the system allows is heavily inspired from Foundry's cheatcodes.

Changelog

Major

  • We now implement most of the components from the ref-fvm repo as we need to have granular control over them for the cheatcode system to work. This implementation is heavily inspired from the conformance test framework from @Stebalian on the ref-fvm
    • KytheraMachine: Stores override values.
    • KytheraCallManager: Set override values when detects a send() syscall to actor Id 98 (cheatcodes actor).
    • KytheraKernel: Actually override values at call time.
  • Introducing an OverrideContext structure that allows us to store at the KytheraMachine level different value to alter information when cheatcodes are invoked.
  • Deploying a Cheatcode Actor at Actor Id 98. It contains entrypoints corresponding to the different cheatcodes we want to be available to our users.
  • Updated the way we compile actors to get binaries. Now, all actors can be found under <project-root>/actors. In this folder we find both actors (available at all time) and test-actors (only available if feature=["testing"] is enabled). This allows to have some binaries built and used in our kythera-lib without actually building everything everytime.

Minor

  • Actor are no longer deployed at a random ID. Instead, we first create an address from their new and register it on-chain.
  • Set default Kythera chain Id to 1312

Links to any relevant issues or pull requests

Closes #22

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

tchataigner and others added 30 commits April 3, 2023 14:39
* Upgrade builtins dependencies to revision for release 10.0 (M2.1 of FVM)
* Using code_by_id() method to retrieve builtins code CID from Manifest

* Get builtins actor ID from builtin crate
* Removed dependencies to builtin actors to rely on Chainsafe repo instead as per filecoin-project/builtin-actors#194 (comment)
* Setup all necessary builtins in our local development environment
* Adding actor to test builtins deployment in Kythera
* Adding actor to test constructor and setup being called in test flow
* Updating the builtin test actor to test that all builtins code are deployed at their address

* Fix clippy
- introduce constructor_setup_test_actor crate to test Setup and
  Constructor functions
Co-authored-by: PhilippeMts <1371613+PhilippeMts@users.noreply.github.com>
Co-authored-by: João Oliveira <hello@jxs.pt>
* Needed to have a Cheatcodes actor incorporated in our library so re-used the kythera_test_actors crate logic to generate it. kythera_test_actors have been renamed to kythera_actors.

* Created a testing feature for kythera_actors to only build test_actors when we need them (mostly during tests of our kythera_lib).
@tchataigner tchataigner requested a review from jxs April 18, 2023 18:37
Copy link
Collaborator

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks for this Thomas, first Round of review, I still need to read the actors and the handle_cheatcode function

Cargo.toml Show resolved Hide resolved
cli/src/utils/search.rs Outdated Show resolved Hide resolved
{
Self {
inner: K::new(
KytheraCallManager(mgr),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file needs all these overrides mainly because of this right? I.e to be able to abstract the CallManager, can we make a comment stating that with a TODO to fix this upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the reasons along with overriding of context in msg_context() and network_context(). So I'm not 100% sure that changing the upstream to remove the passing of the call manager will actually have an impact on our need to do what's currently being implemented (as we are actually overriding the logic of some methods).

What's your take on that ?

fvm/src/machine.rs Show resolved Hide resolved
fvm/src/utils.rs Outdated
pub(crate) const PRANK_METHOD: &str = "Prank";
pub(crate) const TRICK_METHOD: &str = "Trick";

lazy_static::lazy_static! {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: OnceCell is going to be stable soon so it probably makes sense to use its crate instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

wdyt of actually calculating them and hardcoding them? We can then have tests for them on this module to assert that they correspond to the strings and then we could match on them with u64's on handle_cheatcode

lib/src/error.rs Show resolved Hide resolved
lib/src/lib.rs Outdated
@@ -26,7 +27,7 @@ mod state_tree;

/// Main interface to test `Actor`s with Kythera.
pub struct Tester {
// Builtin actors root Cid used in the Machine
// Builtin test_actors root Cid used in the Machine
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the builtin actors test actors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No ! Good one, must have been a wrong update on my side. Thanks for that 🙏

lib/src/state_tree.rs Outdated Show resolved Hide resolved
tchataigner and others added 2 commits April 20, 2023 15:33
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
fvm/src/utils.rs Outdated
pub(crate) const PRANK_METHOD: &str = "Prank";
pub(crate) const TRICK_METHOD: &str = "Trick";

lazy_static::lazy_static! {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wdyt of actually calculating them and hardcoding them? We can then have tests for them on this module to assert that they correspond to the strings and then we could match on them with u64's on handle_cheatcode

cli/src/utils/search.rs Outdated Show resolved Hide resolved
cli/src/utils/search.rs Outdated Show resolved Hide resolved
tchataigner and others added 3 commits April 20, 2023 18:06
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
@tchataigner tchataigner requested a review from jxs April 20, 2023 16:34
jxs
jxs previously approved these changes Apr 21, 2023
Copy link
Collaborator

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM Thomas, only two minor comments left.

lib/src/state_tree.rs Outdated Show resolved Hide resolved
/// Deploy a new Actor at a given address, provided with a given token balance
/// and returns the CodeCID of the installed actor
pub fn deploy_actor_from_bin(
fn deploy_actor_from_bin_at_address(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should still have a comment no?

@tchataigner tchataigner requested a review from jxs April 21, 2023 12:02
jxs
jxs previously approved these changes Apr 21, 2023
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