feat: allow test utils to use builds from outside the RPC repo root #543

bajpai244 · 2023-09-08T09:19:59Z

Time spent on this PR:

Resolves: #542

Pull Request type

Please check the type of change your PR introduces:

What is the new behavior?

The current change will allow the RPC to consume builds outside from the project root of Kakarot RPC { as of now we need this to simplify CI flow for running ef-tests on kakarot main }, we still get all good DX features we used to have, the flow now is:

search for the path as absolute path.
if the above fails, then search for it relative to the project root.

A new environment variable FOUNDRY_OUTPUT_PATH has been added so that we can point to solidity builds outside from this repo, this will add no extra complexity as of now, the default value here will work out of the box with the current flow until a change is needed.

Does this introduce a breaking change?

Yes
No

crates/core/src/test_utils/deploy_helpers.rs

greged93

one remark

greged93 · 2023-09-08T09:33:11Z

crates/core/src/test_utils/deploy_helpers.rs

@@ -98,12 +98,29 @@ pub fn get_contract(filename: &str) -> CompactContractBytecode {
    let dot_sol = format!("{filename}.sol");
    let dot_json = format!("{filename}.json");

-    let foundry_default_out = load_config().out;


can we keep the load config and use your method if there is not config? It makes more sense imo to use to config if you find one

So, I think we should avoid this, because:

we would then need more logic, i.e checking whether FOUNDRY_OUTPUT_PATH exists or not, there are scenarious where someone might have forget to provide this variable, but RPC will still take the builds from local builds, this can create confusion, as they might not be suspecting that they have forgot to provide this variable. { TLDR; I think it will make it more automagical which we don't seem to need as of now, this is used in just one place, if complexity increases, might we can do this. }

Eikix

It's not completely clear to me what we're doing in this PR
Is this because we'll run this repo in the Kakarot v0 context, so it's nonsensical to expect Kakarot to point to Kakarot-RPC that points as submodule Kakarot?

Could you add more comments to explain what relative and absolute mean in your context of this PR / CI Epic and maybe with more concrete context or examples.

That way when we read it back in a month we understand

codecov · 2023-09-08T09:41:26Z

Codecov Report

Patch coverage: 83.33% and project coverage change: +49.29% 🎉

Comparison is base (71506a8) 23.17% compared to head (4efddee) 72.47%.
Report is 213 commits behind head on main.

❗ Current head 4efddee differs from pull request most recent head 510fe20. Consider uploading reports for the commit 510fe20 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #543       +/-   ##
===========================================
+ Coverage   23.17%   72.47%   +49.29%     
===========================================
  Files           9       44       +35     
  Lines        1247     4014     +2767     
===========================================
+ Hits          289     2909     +2620     
- Misses        958     1105      +147

Files Changed	Coverage Δ
crates/core/src/mock/serde.rs	`0.00% <0.00%> (ø)`
crates/core/src/models/transaction.rs	`93.63% <ø> (ø)`
crates/core/src/models/transaction_receipt.rs	`97.02% <ø> (ø)`
crates/core/src/test_utils/bin/dump-katana.rs	`0.00% <ø> (ø)`
crates/core/src/test_utils/deploy_helpers.rs	`33.02% <ø> (ø)`
crates/core/src/test_utils/execution_helpers.rs	`100.00% <ø> (ø)`
crates/core/src/test_utils/fixtures.rs	`100.00% <ø> (ø)`
crates/eth-rpc/src/api/alchemy_api.rs	`100.00% <ø> (ø)`
crates/eth-rpc/src/api/eth_api.rs	`100.00% <ø> (ø)`
crates/eth-rpc/src/api/net_api.rs	`100.00% <ø> (ø)`
... and 34 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bajpai244 · 2023-09-08T10:11:19Z

@Eikix so, the scenarios is:

We have a CI job, where we build a recent PR of Kakarot
Now, we want to:
- build this new version of kakarot { which will be outside of the Kakarot RPC repo }
- create a katana dump on the RPC
- run ef tests on this new dump
Kakarot RPC as of now search for builds relative to its project path root, this makes it difficult to point it to use builds outside from the repo.

One option is to put builds inside Kakarot RPC, but when you do that, then you realize it is better to make this logic more generic on Kakarot RPC itself!

Does this helps in understanding the use case?

Concrete example

We build kakarot on CI { path let's say -> ~/kakarot/build }
We clone Kakarot RPC to make the dump { path let's say -> ~/kakarot-rpc }
We want to use { ~/kakarot/build on the ~/kakarot-rpc/ to create katana dump }

Since the RPC will look for this folder relative to its root, the solution the only solution that can work as of now is to move ~/kakarot/build to ~/kakarot-rpc, this should not be the behaviour.

Also, since as of now to load solidity contracts, we use the foundry.toml file, it makes pointing to some other directory for using contracts very difficult.

This PR is keeping all that used to work before this alongwith also adding the functionality of making this more generic.

greged93 · 2023-09-08T12:10:14Z

I think this might be overkill and cause more complexity in the already tangled up deploy helpers.

Could we maybe untangle the deploy helpers a bit and check if there isn't a cleaner way to do this? We can make a code smell issue to clean the config loading. I see for example a foundry_toml_dirs function which might be useful.

As discussed with @ClementWalter we might want to pause rolling out anything new a little and work on cleaning knots/smells. I think having ef-tests in Kakarot CI is important but this might be a little rushed. Just my opinion.

bajpai244 · 2023-09-08T12:46:07Z

@greged93 I agree with you here, but I was thinking might be we can for now proceed with this as ef-tests on Kakarot CI are important and this is doesn't change much in terms of DX we already have.

We can later fix this as a code smell issue, and once that is done we can make a small change to the CI file as well!

Lemme know what you think, if you still think we should do a different refactor first, then I am fine with that as well.

Eikix

some comments about clarity of the two different cases

Eikix · 2023-09-11T05:25:23Z

crates/core/src/test_utils/deploy_helpers.rs

+             `./scripts/make_with_env.sh test`",
+        );
+
+        // We search firstly for existence for absolute path


this comment isn't so clear, absolute path for what, could we provide an example of path, and in which case it's used?

Ah, so by comments you mean comments on the code, lemme make this better!

Eikix · 2023-09-11T05:27:17Z

crates/core/src/test_utils/deploy_helpers.rs

+         `./scripts/make_with_env.sh test`",
+    );
+
+    // We search firstly for existence for absolute path


same remark as above

Eikix · 2023-09-11T05:27:23Z

crates/core/src/test_utils/deploy_helpers.rs

+    if compiled_kakarot_path_absolute.try_exists().unwrap() {
+        compiled_kakarot_path_absolute
+    } else {
+        // If the absolute path is not found, then we search for a path relative to the project root


same remark

updated, lemme know if this works.

Eikix · 2023-09-11T05:27:30Z

crates/core/src/test_utils/deploy_helpers.rs

+        if foundry_output_path_absolute.try_exists().unwrap() {
+            foundry_output_path_absolute
+        } else {
+            // If the absolute path is not found, then we search for a path relative to the project root


same remark

updated, lemme know if this works.

greged93 · 2023-09-11T05:39:58Z

@bajpai244 I agree that they are important but we didn't have them for 8 months, I would rather delayed them for 10 more days and integrate them nicely then increase the knots in the deploy helper.

Eikix · 2023-09-11T06:05:38Z

Looking at the big picture, I'd ask for us to at least sit down for a second and look at the Kakarot cairo0 repo, and think how can we make the CI run smoothly:

run ef tests
run kakarot rpc tests

This requires a smooth dump management as you pointed out Harsh and maybe some refactors to be able to more freely handle build paths etc.

But i'd argue we need to think in terms of architecture first

ClementWalter · 2023-09-12T13:59:07Z

.env.example

@@ -24,6 +24,7 @@ PROXY_ACCOUNT_CLASS_HASH=0xba8f3f34eb92f56498fdf14ecac1f19d507dcc6859fa6d85eb854

 ## configurations for testing
 COMPILED_KAKAROT_PATH=lib/kakarot/build
+FOUNDRY_OUTPUT_PATH=./lib/kakarot/tests/integration/solidity_contracts/build


do we need ./ or not? see it's not put for COMPILED_KAKAROT_PATH ; when adding similar context into a file, make sure that you stay consistent

ClementWalter · 2023-09-12T14:12:59Z

crates/core/src/test_utils/deploy_helpers.rs

@@ -423,14 +443,31 @@ pub fn compute_kakarot_contracts_class_hash() -> Vec<(String, FieldElement)> {
        .collect()
 }

+fn compiled_kakarot_path() -> PathBuf {


so this is kakarot build dir right? when the path is a directory, it's more explicit to use directory

in the beginning I thought it was the full path like kakarot/build/kakarot.json

ClementWalter · 2023-09-12T14:13:58Z

crates/core/src/test_utils/deploy_helpers.rs

-    let paths = fs::read_dir(&compiled_kakarot_path)
+    let compiled_kakarot_path = &compiled_kakarot_path();
+
+    let paths = fs::read_dir(compiled_kakarot_path)


so these paths are build artifacts?

Eikix · 2023-09-15T07:51:15Z

Should this be closed?

bajpai244 · 2023-09-19T06:09:12Z

@Eikix this can be closed, with the new refactors we are doing this PR shouldn't be much needed anymore.

Adds transfer of value when executing `CALLCODE`.  Time spent on this PR: 0.2 days ## Pull request type  Please check the type of change your PR introduces: - [ ] Bugfix - [x] Feature - [ ] Code style update (formatting, renaming) - [ ] Refactoring (no functional changes, no api changes) - [ ] Build related changes - [ ] Documentation content changes - [ ] Other (please describe): ## What is the current behavior?  Executing `CALLCODE` does not transfer any value to the target `account`. Resolves kkrt-labs#543 ## What is the new behavior?  - `CALLCODE` transfers `value` to `account`. - Additional unit testing for `CALLCODE` opcode value transfer.

bajpai244 requested review from ClementWalter and Eikix as code owners September 8, 2023 09:19

bajpai244 force-pushed the feat/remove-use-of-root_project_path-for-compiled-kakarot-path branch from 2279d16 to bc75858 Compare September 8, 2023 09:27

feat: allow test utils to use builds from outside the RPC repo root

4efddee

bajpai244 force-pushed the feat/remove-use-of-root_project_path-for-compiled-kakarot-path branch from bc75858 to 4efddee Compare September 8, 2023 09:29

Eikix reviewed Sep 8, 2023

View reviewed changes

crates/core/src/test_utils/deploy_helpers.rs Show resolved Hide resolved

greged93 reviewed Sep 8, 2023

View reviewed changes

Eikix requested changes Sep 8, 2023

View reviewed changes

Eikix reviewed Sep 11, 2023

View reviewed changes

dev: improve comments

510fe20

ClementWalter reviewed Sep 12, 2023

View reviewed changes

Eikix closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow test utils to use builds from outside the RPC repo root #543

feat: allow test utils to use builds from outside the RPC repo root #543

bajpai244 commented Sep 8, 2023 •

edited

Loading

greged93 left a comment

greged93 Sep 8, 2023

bajpai244 Sep 8, 2023 •

edited

Loading

Eikix left a comment

codecov bot commented Sep 8, 2023 •

edited

Loading

bajpai244 commented Sep 8, 2023

greged93 commented Sep 8, 2023 •

edited

Loading

bajpai244 commented Sep 8, 2023

Eikix left a comment

Eikix Sep 11, 2023

bajpai244 Sep 11, 2023

Eikix Sep 11, 2023

Eikix Sep 11, 2023

bajpai244 Sep 11, 2023

Eikix Sep 11, 2023

bajpai244 Sep 11, 2023

greged93 commented Sep 11, 2023

Eikix commented Sep 11, 2023

ClementWalter Sep 12, 2023

ClementWalter Sep 12, 2023

ClementWalter Sep 12, 2023

ClementWalter Sep 12, 2023

Eikix commented Sep 15, 2023

bajpai244 commented Sep 19, 2023

feat: allow test utils to use builds from outside the RPC repo root #543

feat: allow test utils to use builds from outside the RPC repo root #543

Conversation

bajpai244 commented Sep 8, 2023 • edited Loading

Pull Request type

What is the new behavior?

Does this introduce a breaking change?

greged93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajpai244 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Eikix left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

bajpai244 commented Sep 8, 2023

Concrete example

greged93 commented Sep 8, 2023 • edited Loading

bajpai244 commented Sep 8, 2023

Eikix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greged93 commented Sep 11, 2023

Eikix commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eikix commented Sep 15, 2023

bajpai244 commented Sep 19, 2023

bajpai244 commented Sep 8, 2023 •

edited

Loading

bajpai244 Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

greged93 commented Sep 8, 2023 •

edited

Loading