-
Notifications
You must be signed in to change notification settings - Fork 71
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
Formatting and Comments #364
Conversation
Does this follow the Orlando standard? |
yep :)) |
// @param target Address of the space contract | ||
// @param function_selector Function selector of the action | ||
// @param calldata Calldata array required for the action | ||
// @param session_public_key The StarkNet session public key that was used to generate the signature |
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 is an external function, should it be sessionPublicKey
? or is it only for function names and not the arguments?
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.
Ive just been reading some OZ examples and they actually seem to use a mixture. eg:
snake case here:
https://github.com/OpenZeppelin/cairo-contracts/blob/893f4b98739bdc1b7d64d0511d96d19f290eef88/src/openzeppelin/token/erc20/presets/ERC20.cairo#L89
but then camel here:
https://github.com/OpenZeppelin/cairo-contracts/blob/893f4b98739bdc1b7d64d0511d96d19f290eef88/src/openzeppelin/account/presets/Account.cairo#L48
Ill reach out to OZ and clarify
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.
what a mess this casing is haha
// We don't need to pad because calling `.address` with starknet.js | ||
// already left pads the address with 0s |
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.
Might be interesting to keep this comment?
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.
Actually, might be worth leaving the padding on-chain so that the signed message is more readable in users wallet.
See this issue: snapshot-labs/sx.js#92
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.
See my comment in the issue you posted :) I think we should still keep the comment! :)
Added comments and some general formatting/cleaning up
executor
toexecution_strategy
as we had a mix of the terms.voting_strategy_params_flat
is aftervoting_strategies
in the constructor args.update_*
functions toset_*
closes #210
closes #367