-
Notifications
You must be signed in to change notification settings - Fork 17
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
NTRN-323 Feat: Add dex pause dao logic #106
Conversation
bbb93d0
to
758a610
Compare
Please attach a successful testrun |
.ok_or(ContractError::Unauthorized {})?; | ||
|
||
let dex_params = get_dex_params(deps, ParamsRequestDex {})?; | ||
// TODO: do this through iteration instead of ifs for future proofing new params |
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.
please resolve it somehow
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.
As written in TODO, I don't like it since we will need to upgrade the contract with changes to params of dex. And because it's not an instant process, it's gonna be a pain.
I would suggest the following.
Convert dex_params
to slice of (string, string) values as well as msg_update_params
, assign it the pause param from the message, and then check that all fields exactly match.
WDYT? Not sure if it's easy
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.
I agree it's a bit annoying. But as far as I know, there is no easy way to iterate through a rust struct with arbitrary keys. We could do this by marshalling with serde but it starts to get pretty ugly.
Also, if new dex params are added we would still have to update the contract to change the allow strategies and struct for retrieving dex params. So I don't think it's terrible
Allow dao to change permissions for the dex.
Related PRs:
neutron-org/neutronjsplus#41
https://github.com/neutron-org/neutron-integration-tests/pull/313/files
Integration test run
https://github.com/neutron-org/neutron-tests/actions/runs/10012028532