Migrate to the 2023_11 edition. #995
Conversation
…eat/migrate-to-latest-edition-#856
There was a problem hiding this comment.
Great work on the migration, Eric! I left some comments. Two more things:
- I believe we need to add
pubto all the listed modules insrc/lib.cairo. You can target your repo and branch in scarb to test this. Currently, nothing's accessible on my end. Let me know if I'm mistaken - We also need to update the syntax in the docs...probably better to do it in a separate PR but IMO this should be a priority before we release
| @@ -46,7 +47,7 @@ mod AccountUpgradeable { | |||
| } | |||
|
|
|||
| #[constructor] | |||
| fn constructor(ref self: ContractState, public_key: felt252) { | |||
| pub(crate) fn constructor(ref self: ContractState, public_key: felt252) { | |||
There was a problem hiding this comment.
We're just using pub(crate) so the tests can access this, right?
There was a problem hiding this comment.
Some of the other presets don't require this with testing. Should we directly test x::constructor in the other presets for consistency (thus making all preset constructors require pub(crate))?
There was a problem hiding this comment.
I think we can address this in the migration to foundry issues, so that we can refactor the tests accordingly to be as consistent as possible (note that the udc for example require the pub keyword in the event struct, because there's no component, and this should be probably addressed as well).
src/tests/access.cairo
Outdated
| @@ -1,5 +1,5 @@ | |||
| mod test_accesscontrol; | |||
| mod test_dual_accesscontrol; | |||
| mod test_dual_ownable; | |||
| mod test_ownable; | |||
| pub(crate) mod test_ownable; | |||
There was a problem hiding this comment.
For organizational purposes, do you think we should create shared or common test files for shared test helpers (like event assertions)? pub(crate)ing this particular test module appears a bit random
There was a problem hiding this comment.
that's a great idea, I refactored the modules a bit to make the common module the only public one. Let me know what you think.
There was a problem hiding this comment.
MUCH much better :) nice work!
Agree, let's get this PR merged first, and then I will go through the docs to check each example. |
There was a problem hiding this comment.
Nice work with the common file additions! I think we just need to remove the constants section in the utils docs since this won't be public anymore (https://docs.openzeppelin.com/contracts-cairo/0.13.0/api/utilities#constants). Other than that, I think this is good to go!
Fixes #856
PR Checklist