Skip to content
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

Refactor settings setters; use snapshots for events #456

Merged
merged 8 commits into from
Jul 25, 2023

Conversation

pscott
Copy link
Contributor

@pscott pscott commented Jul 13, 2023

  1. Removes all set* functions
  2. Introduces the update_settings function, similarly to sx-evm.
  3. Updates events to use @ (snapshot) rather than values.

Closes #427

@pscott pscott requested a review from Orland0x July 13, 2023 15:29
@pscott
Copy link
Contributor Author

pscott commented Jul 13, 2023

formatting failed but running cairo-format **/**.cairo doesn't work because I'm guessing some things have not been updated with v2... have to investigate :p

authenticators_to_add: NoUpdateArray::no_update(),
authenticators_to_remove: NoUpdateArray::no_update(),
voting_strategies_to_add: NoUpdateArray::no_update(),
voting_strategies_metadata_URIs_to_add: array::ArrayTrait::new(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use NoUpdateArray::no_update()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was a miss; it's because at first I wanted to have something visually different to remind me that NoUpdateString::no_update() has not been implemented yet...

We can go with empty for now but I think we'll need to change in the future because "" is a valid metadata_uri and I'd be happier with something like No Update as a string to indicate it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can change to NoUpdateArray::no_update() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aye! will do

@Orland0x
Copy link
Contributor

formatting failed but running cairo-format **/**.cairo doesn't work because I'm guessing some things have not been updated with v2... have to investigate :p

might be that cairo-format is different to scarb fmt we use the latter in the CI. i used scarb fmt and it worked

@Orland0x Orland0x merged commit 3bbb8fd into feat_space Jul 25, 2023
2 checks passed
@Orland0x Orland0x deleted the refactor_settings_setters branch July 25, 2023 12:28
@Orland0x Orland0x restored the refactor_settings_setters branch July 25, 2023 12:29
@pscott pscott mentioned this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants