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 method to remove delegate with provided signature #981

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

falvaradorodriguez
Copy link
Contributor

Closes #750

@falvaradorodriguez falvaradorodriguez requested a review from a team as a code owner May 16, 2024 12:17
@Uxio0
Copy link
Member

Uxio0 commented May 16, 2024

@moisses89 when this is ready could you review it?

@coveralls
Copy link

coveralls commented May 16, 2024

Pull Request Test Coverage Report for Build 9365290507

Details

  • 32 of 32 (100.0%) changed or added relevant lines in 2 files are covered.
  • 29 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.1%) to 93.482%

Files with Coverage Reduction New Missed Lines %
gnosis/eth/clients/sourcify_client.py 13 70.59%
gnosis/eth/tests/clients/test_sourcify_client.py 16 64.44%
Totals Coverage Status
Change from base Build 9363150402: -0.1%
Covered Lines: 8175
Relevant Lines: 8745

💛 - Coveralls

moisses89
moisses89 previously approved these changes May 20, 2024
Copy link
Member

@moisses89 moisses89 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@moisses89 moisses89 dismissed their stale review May 20, 2024 12:10

I was missing one thing

@@ -305,6 +305,37 @@ def remove_delegate(
raise SafeAPIException(f"Cannot remove delegate: {response.content}")
return True

def remove_delegate_signed(
Copy link
Member

@moisses89 moisses89 May 20, 2024

Choose a reason for hiding this comment

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

I don't like to have two functions to remove delegates. We have here two options in my opinion.

  • Remove the function that is signing inside
  • or remove_delegate with optional signature and optional signature parameters as the signer.

I prefer the first one, because in my opinion calculate the signature is out of the goal of a client API function.
For both options we should set the label breaking_change for the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, makes sense. Removed here a7eb692

@falvaradorodriguez falvaradorodriguez added the breaking_change Breaking change label May 28, 2024
@falvaradorodriguez
Copy link
Contributor Author

@moisses89 Seeing this it made sense to me to apply the same criteria for the add_delegate method. I have also put an optional safe parameter as in the case of delete. If you see that this doesn't apply here, please let me know and I'll revert it. Thanks

@falvaradorodriguez falvaradorodriguez merged commit b95aba6 into main Jun 5, 2024
7 checks passed
@falvaradorodriguez falvaradorodriguez deleted the remove_delegate_signed branch June 5, 2024 07:42
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking_change Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove delegate API client shouldn't sign the message
4 participants