-
Notifications
You must be signed in to change notification settings - Fork 166
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
Get features from all targets #2570
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only reviewed the logic
[nit] name_for_package
is used only in a unittest, we can remove it
[nit] please move public stuff up in this file and private stuff below it, will be much easier to read
Ofc should be in a separate PR
crates/forge/tests/data/targets/custom_target_custom_names/metadata.json
Outdated
Show resolved
Hide resolved
crates/scarb-api/src/lib.rs
Outdated
// TODO use const | ||
let base_artifacts = contracts_paths | ||
.iter() | ||
.find(|paths| paths.test_type == Some("integration".to_string())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this prioritize None
i.e. contracts from starknet-contract
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic was changed. But there's no case where we have contracts both from "starknet-contract" and from "integration". It's either first if we build with scarb build
or the second when using scarb build --test
.
crates/scarb-api/src/lib.rs
Outdated
for artifact in other_artifacts { | ||
let artifact = load_contracts_artifacts_and_source_sierra_paths(&artifact.path)?; | ||
for (key, value) in artifact { | ||
base_artifacts.entry(key).or_insert(value); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly, this is only meant to be a fallback?
If so, isn't it extremely wasteful, that it compiles all contract files with USC before checking if you even need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. I still think it should be possible to compile all contract files in parallel with multiple usc processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this part is not optimised very well. Imo worth a separate task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use par_iter
to compile these in parallel, it should be safe.
Co-authored-by: Piotr Magiera <56825108+piotmag769@users.noreply.github.com>
…ts' into 2568-get-features-from-all-targets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks fine
crates/scarb-api/src/lib.rs
Outdated
.other_files | ||
.par_iter() | ||
.map(load_contracts_artifacts_and_source_sierra_paths) | ||
.collect::<Result<Vec<_>>>()?; | ||
|
||
for artifact in compiled_artifacts { | ||
for (key, value) in artifact { | ||
base_artifacts.entry(key).or_insert(value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still compiles those files, just to most probably discard them anyway? 👀
Can't we check if we need specific contract artifact without compiling it first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think we can do that. I'll change it so it only compiles artifacts which names' didn't occur before.
crates/scarb-api/src/lib.rs
Outdated
) -> Result<HashMap<String, (StarknetContractArtifacts, Utf8PathBuf)>> { | ||
let mut base_artifacts = load_contracts_artifacts_and_source_sierra_paths(&self.base_file)?; | ||
|
||
let compiled_artifacts = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. is scarb-api
the right place to hide USC calls? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not. We'll have to change it eventually.
<!-- Reference any GitHub issues resolved by this PR --> Closes # ## Introduced changes <!-- A brief description of the changes --> Part of stack Base: #2570 ## Checklist <!-- Make sure all of these are complete --> - [x] Linked relevant issue - [x] Updated relevant documentation - [x] Added relevant tests - [x] Performed self-review of the code - [x] Added changes to `CHANGELOG.md`
let balance = dispatcher.get_balance(); | ||
assert(balance == 0, 'balance == 0'); | ||
|
||
dispatcher.increase_balance(100); | ||
|
||
let balance = dispatcher.get_balance(); | ||
assert(balance == 100, 'balance == 100'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] shouldn't error message indicate what was wrong?
assert(balance == 100, 'balance != 100');
Closes #2568
Closes #2567
Introduced changes
Checklist
CHANGELOG.md