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

Feat/3537 arbitrary via multi sig #3979

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Conversation

Nortsova
Copy link
Contributor

@Nortsova Nortsova commented Dec 18, 2024

Description

This issue covers the multi-sig flow of creating arbitrary transaction.

New saga needs to be added that should accept a target contract address and a list of arbitrary transactions, consisting of function name and a list of arguments. It should create a multi-sig motion to call makeArbitraryTransactions on the Colony contract.

Note

Please Review block-ingestor PR too - JoinColony/block-ingestor#306

Testing

Step 1. Install Multi-Sig extension http://localhost:9091/planex/extensions/MultisigPermissions
Step 2. Give leela and amy owner Multi-Sig roles:
image

Step 3. Create Custom transaction action ("Manage Colony" -> "Custom transactions")

Step 4. Create transaction with the following (or you can use beautiful approach of custom contract that Jakub described here: #3966 )
Contract address: 0xeF841fe1611ce41bFCf0265097EFaf50486F5111
ABI:

[{"type":"constructor","payable":false,"inputs":[{"type":"string","name":"_name"},{"type":"string","name":"_symbol"},{"type":"uint8","name":"_decimals"}]},{"type":"event","anonymous":false,"name":"Approval","inputs":[{"type":"address","name":"src","indexed":true},{"type":"address","name":"guy","indexed":true},{"type":"uint256","name":"wad","indexed":false}]},{"type":"event","anonymous":false,"name":"Burn","inputs":[{"type":"address","name":"guy","indexed":true},{"type":"uint256","name":"wad","indexed":false}]},{"type":"event","anonymous":false,"name":"LogSetAuthority","inputs":[{"type":"address","name":"authority","indexed":true}]},{"type":"event","anonymous":false,"name":"LogSetOwner","inputs":[{"type":"address","name":"owner","indexed":true}]},{"type":"event","anonymous":false,"name":"MetaTransactionExecuted","inputs":[{"type":"address","name":"user","indexed":false},{"type":"address","name":"relayerAddress","indexed":false},{"type":"bytes","name":"functionSignature","indexed":false}]},{"type":"event","anonymous":false,"name":"Mint","inputs":[{"type":"address","name":"guy","indexed":true},{"type":"uint256","name":"wad","indexed":false}]},{"type":"event","anonymous":false,"name":"Transfer","inputs":[{"type":"address","name":"src","indexed":true},{"type":"address","name":"dst","indexed":true},{"type":"uint256","name":"wad","indexed":false}]},{"type":"function","name":"DOMAIN_SEPARATOR","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"bytes32"}]},{"type":"function","name":"PERMIT_TYPEHASH","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"bytes32"}]},{"type":"function","name":"allowance","constant":true,"stateMutability":"view","payable":false,"inputs":[{"type":"address","name":"src"},{"type":"address","name":"guy"}],"outputs":[{"type":"uint256"}]},{"type":"function","name":"approve","constant":false,"payable":false,"inputs":[{"type":"address","name":"guy"},{"type":"uint256","name":"wad"}],"outputs":[{"type":"bool"}]},{"type":"function","name":"authority","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"address"}]},{"type":"function","name":"balanceOf","constant":true,"stateMutability":"view","payable":false,"inputs":[{"type":"address","name":"src"}],"outputs":[{"type":"uint256"}]},{"type":"function","name":"burn","constant":false,"payable":false,"inputs":[{"type":"uint256","name":"wad"}],"outputs":[]},{"type":"function","name":"burn","constant":false,"payable":false,"inputs":[{"type":"address","name":"guy"},{"type":"uint256","name":"wad"}],"outputs":[]},{"type":"function","name":"decimals","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"uint8"}]},{"type":"function","name":"executeMetaTransaction","constant":false,"stateMutability":"payable","payable":true,"inputs":[{"type":"address","name":"_user"},{"type":"bytes","name":"_payload"},{"type":"bytes32","name":"_sigR"},{"type":"bytes32","name":"_sigS"},{"type":"uint8","name":"_sigV"}],"outputs":[{"type":"bytes"}]},{"type":"function","name":"getMetatransactionNonce","constant":true,"stateMutability":"view","payable":false,"inputs":[{"type":"address","name":"_user"}],"outputs":[{"type":"uint256","name":"nonce"}]},{"type":"function","name":"locked","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"bool"}]},{"type":"function","name":"mint","constant":false,"payable":false,"inputs":[{"type":"address","name":"guy"},{"type":"uint256","name":"wad"}],"outputs":[]},{"type":"function","name":"mint","constant":false,"payable":false,"inputs":[{"type":"uint256","name":"wad"}],"outputs":[]},{"type":"function","name":"name","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"string"}]},{"type":"function","name":"nonces","constant":true,"stateMutability":"view","payable":false,"inputs":[{"type":"address","name":"_user"}],"outputs":[{"type":"uint256","name":"nonce"}]},{"type":"function","name":"owner","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"address"}]},{"type":"function","name":"permit","constant":false,"payable":false,"inputs":[{"type":"address","name":"owner"},{"type":"address","name":"spender"},{"type":"uint256","name":"value"},{"type":"uint256","name":"deadline"},{"type":"uint8","name":"v"},{"type":"bytes32","name":"r"},{"type":"bytes32","name":"s"}],"outputs":[]},{"type":"function","name":"setAuthority","constant":false,"payable":false,"inputs":[{"type":"address","name":"authority_"}],"outputs":[]},{"type":"function","name":"setOwner","constant":false,"payable":false,"inputs":[{"type":"address","name":"owner_"}],"outputs":[]},{"type":"function","name":"symbol","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"string"}]},{"type":"function","name":"totalSupply","constant":true,"stateMutability":"view","payable":false,"inputs":[],"outputs":[{"type":"uint256"}]},{"type":"function","name":"transfer","constant":false,"payable":false,"inputs":[{"type":"address","name":"dst"},{"type":"uint256","name":"wad"}],"outputs":[{"type":"bool"}]},{"type":"function","name":"transferFrom","constant":false,"payable":false,"inputs":[{"type":"address","name":"src"},{"type":"address","name":"dst"},{"type":"uint256","name":"wad"}],"outputs":[{"type":"bool"}]},{"type":"function","name":"unlock","constant":false,"payable":false,"inputs":[],"outputs":[]},{"type":"function","name":"verify","constant":true,"stateMutability":"view","payable":false,"inputs":[{"type":"address","name":"_user"},{"type":"uint256","name":"_nonce"},{"type":"uint256","name":"_chainId"},{"type":"bytes","name":"_payload"},{"type":"bytes32","name":"_sigR"},{"type":"bytes32","name":"_sigS"},{"type":"uint8","name":"_sigV"}],"outputs":[{"type":"bool"}]}]

Step 5. Select Decision Method: Multi-Sig
Step 6. Submit form
Step 7. Ensure that Multi-Sig motion was created:
image
Step 8. Login as amy from incognito window and approve motion
Step 9. Finalize it
Step 10. Verify that you can see Incoming funds (that was minted because of the Contract call)
image

Step 11. Repeat Step 3 - Step 7.
Step 12. Remove your vote and reject motion
Step 13. Verify that there is no new income funds (means no Contract was called)

Contributes to #3537

@Nortsova Nortsova self-assigned this Dec 18, 2024
@Nortsova Nortsova requested a review from a team as a code owner December 18, 2024 19:46
@Nortsova Nortsova marked this pull request as draft December 18, 2024 19:47
@Nortsova Nortsova force-pushed the feat/3536-arbitrary-via-reputation branch from 5819a12 to 71cc9f1 Compare December 19, 2024 15:19
@Nortsova Nortsova force-pushed the feat/3537-arbitrary-via-multi-sig branch from f6bb1e2 to 2119e50 Compare December 19, 2024 23:30
@Nortsova Nortsova changed the title [WIP]: Feat/3537 arbitrary via multi sig Feat/3537 arbitrary via multi sig Dec 19, 2024
@Nortsova Nortsova marked this pull request as ready for review December 19, 2024 23:31
mmioana
mmioana previously approved these changes Dec 20, 2024
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Impressive work on adding Custom transactions via Multi-sig @Nortsova 🥇

Installed the Multi-sig extension
Screenshot 2024-12-20 at 10 19 24

Gave leela and amy Owner permissions in General for Multi-sig actions
Screenshot 2024-12-20 at 10 22 39
Screenshot 2024-12-20 at 10 23 09

Created a Custom transactions multi-sig motion
Screenshot 2024-12-20 at 10 23 52
Approved as leela
Screenshot 2024-12-20 at 10 24 02
And as amy
Screenshot 2024-12-20 at 10 24 56

And could see the incoming funds after this motion was finalised
Screenshot 2024-12-20 at 10 25 10

Then created another Custom transactions via multi-sig that I have rejected
Screenshot 2024-12-20 at 10 26 16

And nothing changed on the incoming funds page
Screenshot 2024-12-20 at 10 26 23

Nice work!!!

davecreaser
davecreaser previously approved these changes Dec 27, 2024
Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Nice work @Nortsova! All the code is looking good and it looks to be working as expected:

After installing the extension and giving permissions I could make the action, vote and finalize:
Screenshot 2024-12-27 at 16 09 18
Screenshot 2024-12-27 at 16 10 28
Screenshot 2024-12-27 at 16 10 54

And there were incoming funds: (a tiny amount 🤣)
Screenshot 2024-12-27 at 16 11 06

And cancelling also worked, without new incoming funds appearing:
Screenshot 2024-12-27 at 16 13 31
Screenshot 2024-12-27 at 16 13 41

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Really nice and simple 👌
Form looking all good ✔️
image
Removing a vote works ✔️
image

Reapproving works ✔️
image

Approving as amy works ✔️
image

And finalization also goes through 😎
image
image

However, I just wanted to raise a question here
image

test is the multi-sig motion and it doesn't have the correct description, should this be added?

is Generic action we don't have information about for the actual contract call OK, or should it be something else?

Will preliminarily request changes, and if it's not in scope of this isssue, I'll just approve.

Overall, great job 💪

@Nortsova Nortsova force-pushed the feat/3536-arbitrary-via-reputation branch from 71cc9f1 to 7db7bd4 Compare January 8, 2025 12:14
@Nortsova Nortsova force-pushed the feat/3537-arbitrary-via-multi-sig branch from 2119e50 to 78911fb Compare January 8, 2025 13:52
@Nortsova
Copy link
Contributor Author

Nortsova commented Jan 9, 2025

Nice catch, @bassgeta . Thank you for your help. Fix for Generic action is here now 🙌

@Nortsova Nortsova requested a review from bassgeta January 9, 2025 11:38
bassgeta
bassgeta previously approved these changes Jan 9, 2025
Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Everything still works, and the actions aren't duplicated anymore, there's just one action in the list!
image

jakubcolony
jakubcolony previously approved these changes Jan 10, 2025
Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

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

Looks great, well done @Nortsova 👍

@bassgeta regarding the action metadata in the activity feed, this will be handled in a separate issue - #3538.

image image image image image

@Nortsova Nortsova force-pushed the feat/3537-arbitrary-via-multi-sig branch from ef18af6 to 40d066f Compare January 10, 2025 09:58
Base automatically changed from feat/3536-arbitrary-via-reputation to master January 13, 2025 10:12
@Nortsova Nortsova dismissed stale reviews from jakubcolony, bassgeta, davecreaser, and mmioana January 13, 2025 10:12

The base branch was changed.

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Re-approving 👍

Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Still all good with me after a quick retest 🚀

Screenshot 2025-01-13 at 11 19 00 Screenshot 2025-01-13 at 11 21 23 Screenshot 2025-01-13 at 11 21 43

Reapproving!

@Nortsova Nortsova force-pushed the feat/3537-arbitrary-via-multi-sig branch from 40d066f to 48851c7 Compare January 13, 2025 12:04
@Nortsova Nortsova merged commit b485ae6 into master Jan 13, 2025
2 checks passed
@Nortsova Nortsova deleted the feat/3537-arbitrary-via-multi-sig branch January 13, 2025 12:08
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.

5 participants