Update contract collection docs #2656
Open
+90
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cptartur
requested review from
ddoktorski,
kkawula and
franciszekjob
as code owners
franciszekjob
requested changes
Nov 13, 2024
| It makes Scarb build all contracts from your package and save them to the `target/{current_profile}` directory | ||
| (read more on [Scarb website](https://docs.swmansion.com/scarb/docs/extensions/starknet/contract-target.html)). | ||
| `snforge` supports two mechanism for collecting contracts used in tests. | ||
| The default depends on Scarb version used and can be controlled with `--no-optimization` flag. |
There was a problem hiding this comment.
nit:
Suggested change
| The default depends on Scarb version used and can be controlled with `--no-optimization` flag. | |
| The default one depends on Scarb version used and can be controlled with `--no-optimization` flag. |
Comment on lines
+6
to
+9
| - If using Scarb version greater or equal to | ||
| 2.8.3, [optimized collection mechanism](contract-collection/new-mechanism.md) is used by default. | ||
| - If using Scarb version below 2.8.3 or using `--no-optimization` flag with | ||
| `snforge test` [old collection mechanism](contract-collection/old-mechanism.md) is used. |
There was a problem hiding this comment.
Suggested change
| - If using Scarb version greater or equal to | |
| 2.8.3, [optimized collection mechanism](contract-collection/new-mechanism.md) is used by default. | |
| - If using Scarb version below 2.8.3 or using `--no-optimization` flag with | |
| `snforge test` [old collection mechanism](contract-collection/old-mechanism.md) is used. | |
| - If using Scarb version >= 2.8.3, [optimized collection mechanism](contract-collection/new-mechanism.md) is used by default. | |
| - If using Scarb version < 2.8.3 or running `snforge test` with `--no-optimization` flag, the [old collection mechanism](contract-collection/old-mechanism.md) is used. |
Comment on lines
+13
to
+14
| > When using Scarb versions older than 2.8.3 it is **not possible** to enable new mechanism. | ||
| > Migrating to new Scarb version is required. |
There was a problem hiding this comment.
Maybe let's just say that new mechanism requires Scarb >= 2.8.3, e.g.
Suggested change
| > When using Scarb versions older than 2.8.3 it is **not possible** to enable new mechanism. | |
| > Migrating to new Scarb version is required. | |
| > Enabling new mechanism **requires** Scarb version >= 2.8.3. |
Comment on lines
+18
to
+25
| | Feature | Old Mechanism | Optimised Mechanism | | ||
| |---------------------------------------------------------|---------------|---------------------| | ||
| | Using contracts from `/src` | ✅ | ✅ | | ||
| | Using contracts from `/tests` | ❌ | ✅ | | ||
| | Using contracts from modules marked with `#[cfg(test)]` | ❌ | ✅ | | ||
| | Using contracts from dependencies | ✅ | ✅ | | ||
| | Contracts more closely resemble ones from real network | ✅ | ❌ | | ||
| | Additional compilation step required (`scarb build`) | ✅ | ❌ | |
There was a problem hiding this comment.
nit: Just stylistic 😅
Suggested change
| | Feature | Old Mechanism | Optimised Mechanism | | |
| |---------------------------------------------------------|---------------|---------------------| | |
| | Using contracts from `/src` | ✅ | ✅ | | |
| | Using contracts from `/tests` | ❌ | ✅ | | |
| | Using contracts from modules marked with `#[cfg(test)]` | ❌ | ✅ | | |
| | Using contracts from dependencies | ✅ | ✅ | | |
| | Contracts more closely resemble ones from real network | ✅ | ❌ | | |
| | Additional compilation step required (`scarb build`) | ✅ | ❌ | | |
| | Feature | Old Mechanism | Optimised Mechanism | | |
| |---------------------------------------------------------|---------------|---------------------| | |
| | Using contracts from `/src` | ✅ | ✅ | | |
| | Using contracts from `/tests` | ❌ | ✅ | | |
| | Using contracts from modules marked with `#[cfg(test)]` | ❌ | ✅ | | |
| | Using contracts from dependencies | ✅ | ✅ | | |
| | Contracts more closely resemble ones from real network | ✅ | ❌ | | |
| | Additional compilation step required (`scarb build`) | ✅ | ❌ | |
| It makes Scarb build all contracts from your package and save them to the `target/{current_profile}` directory | ||
| (read more on [Scarb website](https://docs.swmansion.com/scarb/docs/extensions/starknet/contract-target.html)). | ||
|
|
||
| Then, `snforge` loads compiled contracts from the package your tests are in, allowing you to declare the contracts in |
There was a problem hiding this comment.
Suggested change
| Then, `snforge` loads compiled contracts from the package your tests are in, allowing you to declare the contracts in | |
| Then, `snforge` loads compiled contracts from the package your tests are located, allowing you to declare the contracts in |
| ## Using External Contracts In Tests | ||
|
|
||
| If you wish to use contracts from your dependencies inside your tests (e.g. an ERC20 token, an account contract), | ||
| you must first make Scarb build them. You can do that by using `build-external-contracts` property in `Scarb.toml`, |
There was a problem hiding this comment.
Key is the correct name per TOML spec.
Suggested change
| you must first make Scarb build them. You can do that by using `build-external-contracts` property in `Scarb.toml`, | |
| you must first make Scarb build them. You can do that by using `build-external-contracts` key in `Scarb.toml`, |
Comment on lines
+3
to
+5
| For the [`declare`](../../appendix/snforge-library/declare.md) to function, snforge must collect and call build on | ||
| contracts in the package. By default, if using Scarb version greater or equal to 2.8.3, snforge will combine test | ||
| collection and contract collection steps. |
There was a problem hiding this comment.
Not sure if it should be:
- ... function to work, snforge must collect...
- ... function, snforge must collect...
Suggested change
| For the [`declare`](../../appendix/snforge-library/declare.md) to function, snforge must collect and call build on | |
| contracts in the package. By default, if using Scarb version greater or equal to 2.8.3, snforge will combine test | |
| collection and contract collection steps. | |
| For the [`declare`](../../appendix/snforge-library/declare.md) function to, snforge must collect and call build on | |
| contracts in the package. By default, if using Scarb >= 2.8.3, snforge will combine test | |
| collection and contract collection steps. |
| target. If `integration` is not present, snforge will first collect contracts from the first encountered `[[test]]` | ||
| target. | ||
|
|
||
| After collecting from initial `[[test]]` target, snforge will collect contracts from any other encountered contracts. |
There was a problem hiding this comment.
snforge will collect contracts from any other encountered contracts.
should this sound like this 😅 ?
|
|
||
| ## Using External Contracts in Tests | ||
|
|
||
| To use contract from dependencies in tests `Scarb.toml` must be updated to include these contracts under |
There was a problem hiding this comment.
nit:
Suggested change
| To use contract from dependencies in tests `Scarb.toml` must be updated to include these contracts under | |
| To use contract from dependencies in tests, `Scarb.toml` must be updated to include these contracts under |
ddoktorski
reviewed
Nov 14, 2024
| | Using contracts from modules marked with `#[cfg(test)]` | ❌ | ✅ | | ||
| | Using contracts from dependencies | ✅ | ✅ | | ||
| | Contracts more closely resemble ones from real network | ✅ | ❌ | | ||
| | Additional compilation step required (`scarb build`) | ✅ | ❌ | |
There was a problem hiding this comment.
With the current version it kind of suggests that having this additional compilation step is better. Wouldn't it be more appropriate to rename it to Less compilation steps required? Then the green mark would be on the optimized side.
| When you call `snforge test`, one of the things that `snforge` does is that it calls Scarb, particularly `scarb build`. | ||
| It makes Scarb build all contracts from your package and save them to the `target/{current_profile}` directory | ||
| (read more on [Scarb website](https://docs.swmansion.com/scarb/docs/extensions/starknet/contract-target.html)). | ||
| `snforge` supports two mechanism for collecting contracts used in tests. |
There was a problem hiding this comment.
Suggested change
| `snforge` supports two mechanism for collecting contracts used in tests. | |
| `snforge` supports two mechanisms for collecting contracts used in tests. |
| ```toml | ||
| [[target.starknet-contract]] | ||
| build-external-contracts = ["path::to::Contract1", "other::path::to::Contract2"] | ||
| ``` |
There was a problem hiding this comment.
I think a note with the link to the Scarb documentation should be added, the same as here
|
|
||
| Contracts are collected from both `src` and `tests` directory, including modules marked with `#[cfg(test)]`. | ||
| Internally, snforge collects contracts from all `[[test]]` targets compiled by Scarb. | ||
| You can read more about that in [test collection](../test-collection.md) documentation. |
There was a problem hiding this comment.
Not clear what this sentence means. It suggests that you can read more about collecting contracts in the test collection documentation, which is not the case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2627
Introduced changes
Checklist
CHANGELOG.md