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

Add param to allow non lock owner to force unlock #4252

Closed
wants to merge 1 commit into from

Conversation

iboss-ptk
Copy link
Contributor

@iboss-ptk iboss-ptk commented Feb 8, 2023

What is the purpose of the change

To allow an account to forcibly unlock any lock, including ones they do not own, if their address is on the whitelist. This functionality is needed for transmigrator

Brief Changelog

  • Add NonOwnerForceUnlockAddresses to lockup params
  • By pass ownership check in ForceUnlock if msg sender is in NonOwnerForceUnlockAddresses

Testing and Verifying

This change added tests and can be verified as follows:

  • Added unit test that validates if msg sender is in NonOwnerForceUnlockAddresses
    • they can force unlock their own locks
    • they can force unlock their other's locks

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
  • How is the feature or change documented? (not documented)

@ValarDragon
Copy link
Member

What is the precise permission we need to expose, this feels too permissive to expose

@iboss-ptk
Copy link
Contributor Author

iboss-ptk commented Feb 8, 2023

@ValarDragon the contract needs to be able to:

unlock -> exit pool -> swap -> join pool -> lock

While retaining deposit period & needs to get everything unlocked locked back in some form.

The problem with existing approach is that if we authz ForceUnlock, msg.Owner will be the account that calls the contract, not the contract itself. If we can get authz MsgExec sender somehow (which is the contract in this case), that might be better approach since it can be more restrictive. I'm not sure if that is possible.

@mattverse
Copy link
Member

mattverse commented Feb 9, 2023

feels like authz is a more preferrable way than using this method to me as well, @iboss-ptk do you need help on further investigation if this is possible?

@iboss-ptk
Copy link
Contributor Author

@mattverse That would be helpful! I'm a bit lost in getting MsgExec sender info without modifying existing MsgForceUnlock. If there is a way for that, that would be ideal.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@sunnya97
Copy link
Collaborator

sunnya97 commented Jul 3, 2023

@mattverse I don't see how authz would help here. Cause normal users don't have ability to force unlock, so they can't authz the ability to do that either

@github-actions github-actions bot removed the Stale label Jul 4, 2023
@sunnya97
Copy link
Collaborator

sunnya97 commented Jul 5, 2023

TODO: Allow breaking of superfluid as well

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Jul 20, 2023
@github-actions github-actions bot closed this Jul 27, 2023
@mattverse mattverse reopened this Jul 27, 2023
@github-actions github-actions bot removed the Stale label Jul 28, 2023
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Aug 12, 2023
@mattverse mattverse removed the Stale label Aug 14, 2023
@p0mvn
Copy link
Member

p0mvn commented Aug 18, 2023

@iboss-ptk are we still planning to pursue this? If not, let's close.

@mattverse mattverse mentioned this pull request Aug 21, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Sep 2, 2023
@github-actions github-actions bot closed this Sep 8, 2023
@p0mvn p0mvn removed their assignment Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants