-
Notifications
You must be signed in to change notification settings - Fork 354
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
Bump starknet foundry to 0.30.0 #1137
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.
Looking good, Gustavo! I left some comments and questions
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.
Looking great @ggonzalez94! I Left just a small suggestion, but besides that, it looks ready to go.
@@ -1,11 +1,16 @@ | |||
use crate::panic_data_to_byte_array; | |||
use snforge_std::{ContractClass, ContractClassTrait}; | |||
use snforge_std::{ContractClass, ContractClassTrait, DeclareResult}; | |||
use starknet::ContractAddress; | |||
|
|||
/// Declares a contract with a `snforge` `declare` call and unwraps the result. | |||
pub fn declare_class(contract_name: ByteArray) -> ContractClass { |
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.
Since this change affects the declare_and_deploy
and declare_and_deploy_at
functions behavior, I think it is worth adding a note to those stating that the function will skip declaration if the contract is already declared. A note on this function could be helpful as well.
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 catch! I just updated the inline docs and the antora docs. Let me know what you think.
I forgot to mention the CHANGELOG. We have to add the corresponding entries:
|
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>
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.
Looks just about ready! I left a couple suggestions and a question
@ericnordelo @andrew-fleming I think this is now ready for a final review and hopefully ready to merge 🤞 |
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.
LGTM!
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.
Great job, Gustavo! I've left a few minor comments, apart from that everything looks good to me
Fixes #1135 and unblocks #1129
snforge_std
to 0.30.0assert_macros
dependency, which is required afterv0.28.0
DeclareResult
enum0x
(e.g. instead of0x06dd34965d008db405187f4ea6170934a051f8ebb4b72c8aba99cb217a281ad4
starknet foundry returns0x6dd34965d008db405187f4ea6170934a051f8ebb4b72c8aba99cb217a281ad4
), so I created a new function in the common test helpersinto_base_16_string_no_padding
, which is the same as its padding counterpart but matches the starknet foundry output. We could remove the other one, but it is part of the public api - so I decided to keep it, but don't have a strong opinion about it.PR Checklist