Insert ordered signatures in Safe Tx #1060
Conversation
Pull Request Test Coverage Report for Build 9381071744Details
💛 - Coveralls |
gnosis/safe/safe_tx.py
Outdated
| self.signatures[: 65 * new_owner_pos] | ||
| + signature | ||
| + self.signatures[65 * new_owner_pos :] | ||
| new_tx_signatures = SafeSignature.parse_signature( |
There was a problem hiding this comment.
Maybe just signatures, because new is not accurated to the variable, I mean the variable contains old signatures and new signatures.
Other option could be unsorted_signatures is you want to remark that are unsorted and needs to be sorted.
There was a problem hiding this comment.
Updated here 5c8cf1e
gnosis/safe/safe_tx.py
Outdated
| tx_signatures = SafeSignature.parse_signature( | ||
| self.signatures, self.safe_tx_hash | ||
| ) | ||
| new_tx_signatures = list(filter(lambda x: x.owner != address, tx_signatures)) |
There was a problem hiding this comment.
I would expect one signature by owner, why returns a list?
Anyway, do you think that make sense change the unsign? in this case remove one signature from the middle does not affect the sort.
There was a problem hiding this comment.
I think the problem is in the name of the method. The signatures stores the signatures of all owners in bytes... that's why you need to parse them to get the list to manage it (In this case, I am calculating the signatures without the signature of the owner to be removed.).
I did it to keep the logic at the same point. In this case SafeSignature. If you see that it doesn't help, I can remove this change, what do you say?
There was a problem hiding this comment.
If I keep it, I must change the name of the variable to be consistent with the above.
There was a problem hiding this comment.
If I keep it, I must change the name of the variable to be consistent with the above.
Done here 314d30b
Pull Request Test Coverage Report for Build 9387031259Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9398016275Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9399810476Details
💛 - Coveralls |
Closes #713
The approach of the task changes due a utility for sorting and exporting signatures has been created in this task #722.
This implies that the responsibility for sorting belongs to this new utility and that it is not necessary to do the task as foreseen.
In the PR only the management that was duplicated in the SafeTx class is removed and the existing SafeSignature is used.