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.

Sign up for GitHub

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

Jump to bottom

Add update settings tests #506

Merged
merged 6 commits into from
Aug 31, 2023
Merged

Add update settings tests #506

merged 6 commits into from
Aug 31, 2023

Conversation

pscott
Copy link
Contributor

@pscott pscott commented Aug 19, 2023

No description provided.

@pscott pscott changed the title [WIP] Add update settings tests Add update settings tests Aug 21, 2023
@pscott pscott marked this pull request as ready for review August 21, 2023 10:25
@pscott pscott requested a review from Orland0x August 21, 2023 10:25
Orland0x
Orland0x reviewed Aug 28, 2023
View reviewed changes
starknet/src/tests/test_update_settings.cairo Outdated

#[test]
#[available_gas(10000000000)]
// #[should_panic(expected: ('Invalid duration', 'ENTRYPOINT_FAILED'))] // TODO: uncomment once PR is merged
Copy link
Contributor

Choose a reason for hiding this comment

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

which PR is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#505 fixed the update_settings function

@Orland0x
Copy link
Contributor

We need to actually store the daoURI in the state in order to be compatible with the standard like we do in sx-evm. I forgot to do this before
https://github.com/snapshot-labs/sx-evm/blob/df37a70673067300a3a6c0541740117a86d0bc59/src/Space.sol#L49C12-L49C12

@pscott
Copy link
Contributor Author

pscott commented Aug 31, 2023 •
edited
Loading

We need to actually store the daoURI in the state in order to be compatible with the standard like we do in sx-evm. I forgot to do this before https://github.com/snapshot-labs/sx-evm/blob/df37a70673067300a3a6c0541740117a86d0bc59/src/Space.sol#L49C12-L49C12

Yup, funny you mention it, I'm on it with this PR, maybe we can merge this one now so I re-update the update_settings function?

@Orland0x
Copy link
Contributor

We need to actually store the daoURI in the state in order to be compatible with the standard like we do in sx-evm. I forgot to do this before https://github.com/snapshot-labs/sx-evm/blob/df37a70673067300a3a6c0541740117a86d0bc59/src/Space.sol#L49C12-L49C12

Yup, funny you mention it, I'm on it with this PR, maybe we can merge this one now so I re-update the update_settings function?

yup cool

Orland0x
Orland0x approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@Orland0x Orland0x left a comment

Choose a reason for hiding this comment

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

uTACK

@Orland0x Orland0x merged commit 1c97753 into develop Aug 31, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Orland0x Orland0x Orland0x approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pscott @Orland0x