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

build: override queue #180

Merged
merged 6 commits into from
Oct 9, 2023
Merged

build: override queue #180

merged 6 commits into from
Oct 9, 2023

Conversation

Schlagonia
Copy link
Collaborator

Description

Let governance override custom withdraw queues

Fixes # (issue)

Checklist

  • I have run vyper and solidity linting
  • I have run the tests on my machine
  • I have followed commitlint guidelines
  • I have rebased my changes to the latest version of the main branch

Copy link
Contributor

@jmonteer jmonteer left a comment

Choose a reason for hiding this comment

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

I think this implementation can lead to issues around unexpected outputs.

if override_queue is used and a custom_queue different than default_queue is passed, it should revert. otherwise a user might expect the vault to withdraw from specific strategies and not get that (while that behaviour being important for some reason, e.g. gas usage).

maybe override_queue should be renamed to allow_custom_queue

but I like the idea

@Schlagonia
Copy link
Collaborator Author

I think this implementation can lead to issues around unexpected outputs.

if override_queue is used and a custom_queue different than default_queue is passed, it should revert. otherwise a user might expect the vault to withdraw from specific strategies and not get that (while that behaviour being important for some reason, e.g. gas usage).

maybe override_queue should be renamed to allow_custom_queue

but I like the idea

I dont think we should revert.

Seems the downside to always reverting is much bigger than servicing a withdraw with a different queue.

I also dont want to iterate over an custom queue to check if it may be the same as the default.

If someone is concerned enough to be passing a custom queue we should expect them to be able to check the public variable to know if the queue will be overriden.

Question is what is the most obvious nameing/bool to know it will be used.

I think "allow_custom_queue" could also be misleading since we would allow it to be sent, its just doesnt get used.

@Schlagonia
Copy link
Collaborator Author

Schlagonia commented Sep 22, 2023

I think this implementation can lead to issues around unexpected outputs.

if override_queue is used and a custom_queue different than default_queue is passed, it should revert. otherwise a user might expect the vault to withdraw from specific strategies and not get that (while that behaviour being important for some reason, e.g. gas usage).

maybe override_queue should be renamed to allow_custom_queue

but I like the idea

I think the bool should be 'false' if custom queues are allowed to be used, since thats the default value and the switched needs to be flipped (turned True) to override the queue.

perhaps 'override_custom_queue' would be more clear?

or 'use_default_queue'

@Schlagonia
Copy link
Collaborator Author

Schlagonia commented Oct 6, 2023

I think this implementation can lead to issues around unexpected outputs.

if override_queue is used and a custom_queue different than default_queue is passed, it should revert. otherwise a user might expect the vault to withdraw from specific strategies and not get that (while that behaviour being important for some reason, e.g. gas usage).

maybe override_queue should be renamed to allow_custom_queue

but I like the idea

changed the name to use_default_queue 1332f5d

@Schlagonia Schlagonia merged commit 42e79e7 into yearn:master Oct 9, 2023
7 checks passed
@Schlagonia Schlagonia deleted the override branch October 9, 2023 23:17
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.

4 participants