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: implement create_engine #14

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Conversation

tamaralipows
Copy link
Contributor

@tamaralipows tamaralipows commented Oct 23, 2024

Task

Unforeseen changes:

  • Needed to factor out the init_account into a Trait implemented by both SimulationDB and TychoDB in order to keep the simulation engine creation generalized. More information in this slack thread.
  • We are not able to eliminate mentions of DatabaseRef here since the SimulationEngine.simulate method requires this trait when initializing the builder
        let default_builder = Evm::builder()
            .with_spec_id(SpecId::CANCUN)
            .with_ref_db(db_ref) <----------------- we cannot change this.
            .with_block_env(block_env)
            .with_tx_env(tx_env);

Notes:

We plan on creating a more thorough integration test after finishing the sprint goals (which will leave us with everything necessary for creating the integration test). This is why we are not wasting too much time integration-testing the engine and ProtoSim/Adapter contracts. The test in the engine for this PR simply checks that the call to init_account succeeds with the expected types.

@tamaralipows tamaralipows marked this pull request as draft October 23, 2024 00:07
@tamaralipows tamaralipows force-pushed the tnl/ENG-3751-create-engine-util branch from f2f7f2a to 0d7bb3c Compare October 24, 2024 19:47
@tamaralipows tamaralipows changed the title [WIP] feat: implement create_engine feat: implement create_engine Oct 24, 2024
@tamaralipows tamaralipows force-pushed the tnl/ENG-3751-create-engine-util branch from 0d7bb3c to 4c3cd0c Compare October 24, 2024 19:57
@tamaralipows tamaralipows marked this pull request as ready for review October 24, 2024 19:57
@tamaralipows tamaralipows force-pushed the tnl/ENG-3751-create-engine-util branch from 7203d5a to 4c3cd0c Compare October 24, 2024 21:34
Copy link
Collaborator

@dianacarvalho1 dianacarvalho1 left a comment

Choose a reason for hiding this comment

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

Thank you so much @tamaralipows 🙏🏼 This looks good, I like it! I only have small questions/suggestions!

Comment on lines +129 to +130
unsafe impl Send for MockDatabase {}
unsafe impl Sync for MockDatabase {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting, why was this needed? What does it breaks if the mock db isn't Send or Sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The usage of Arc for the test:

warning: usage of an `Arc` that is not `Send` and `Sync`
   --> src/protocol/vm/engine.rs:138:18
    |
138 |         let db = Arc::new(RwLock::new(MockDatabase::default()));
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `Arc<RwLock<MockDatabase>>` is not `Send` and `Sync` as `RwLock<MockDatabase>` is not `Sync`
    = help: if the `Arc` will not used be across threads replace it with an `Rc`
    = help: otherwise make `RwLock<MockDatabase>` `Send` and `Sync` or consider a wrapper type such as `Mutex`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
    = note: `#[warn(clippy::arc_with_non_send_sync)]` on by default

Copy link
Contributor

@zizou0x zizou0x Oct 30, 2024

Choose a reason for hiding this comment

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

FYI, it's not send and sync because you used a RefCell here

If you change this to be something like a Mutex then you don't need this unsafe code

Also please note that these two lines are really dangerous and should never be used in the program code

- Change database.rs -> simulation_db.rs for clarity.
- Make EXTERNAL_ACCOUNT already an address, so we don't need to re-convert it in the engine creation
- remove supposedly unnecessary `auto_impl`
Copy link
Collaborator

@dianacarvalho1 dianacarvalho1 left a comment

Choose a reason for hiding this comment

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

Yeeesss looks good 🙏🏼 ✨ Thank you thank you!

@tamaralipows tamaralipows merged commit 0bcbf64 into main Oct 25, 2024
3 checks passed
@tamaralipows tamaralipows deleted the tnl/ENG-3751-create-engine-util branch October 25, 2024 17:02
@propellerci
Copy link

propellerci bot commented Oct 25, 2024

This PR is included in version 0.25.0 🎉

@propellerci propellerci bot added the true label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants