Refactor dao acl - #858 task

[dapplion]

🦅 Pull Request

Subtask of #858. Separate the logic from the CLI front-end and test the logic part.

• acl create command
• acl grant command
• acl remove-manager command
• acl revoke command
• acl set-manager command
• acl view command

acl view:
Split the logic into three files / parts

• lib/acl/view: Fetches the data and interfaces with aragon-js

[ "0xab1200000000000000000000000000000000cd34"  ]

• lib/acl/viewFormatter: Parses the data and returns the data relevant to the user appended with human names. This component is fully tested

{ name: "acl", address: "0xab1200000000000000000000000000000000cd34" }

• commands/dao/acl/view (original): Call data fetcher, call parser, and then print the data in as a table with emojis on other stringified content.

acl (0xab12...cd34)

Also, every used function is now typed with JSDoc, and the typings abstracted to typedef.js. I have seen that @macor161 also uses JSDoc on new code, can we create general type definitions for reused that structures such a module, network etc?

others:
All other dao acl commands just called the wrapper execHandler and since they do just one single asynchronous task there is no intermediate logging. If its logic was moved out to a lib/**/* file I'm not sure if it would just be code duplication. After further review, there are too many dependencies of these utils, so I can't safely refactor them at the moment. I would recommend considering the exclusive acl commands refactored and tackle the dao utils refactors latter.

🚨 Test instructions

Run aragon dao acl commands against your DAO

✔️ PR Todo

• Include links to related issues/PRs
• Update unit tests for this change
• Update the relevant documentation
• Clear dependencies on other modules that have to be released before merging this

[mcormier]

Hey. I was mentioned above but this project does not remind me of anything. Did I make a suggestion a long time ago that is now being considered or was the wrong person added in the description? Cheers.

[0xGabi]

Hi @mcormier sorry about that we have a team mate with a similar GitHub username

[dapplion]

Yep, I mistaken you for @macor161. Thank you @0xGabi for amending the PR mentions.

[0xGabi]

LGTM 🙌

@macor161 can you double check the typedef files before we merge it.

I'll create an issue to track the abstraction options for the execHandler

[codecov]

Codecov Report

Merging #864 into develop will increase coverage by 0.69%.
The diff coverage is 24.63%.

@@            Coverage Diff             @@
##           develop    #864      +/-   ##
==========================================
+ Coverage     7.13%   7.83%   +0.69%     
==========================================
  Files           89      91       +2     
  Lines         2423    2425       +2     
==========================================
+ Hits           173     190      +17     
+ Misses        2250    2235      -15

Impacted Files	Coverage Δ
packages/aragon-cli/src/lib/acl/view.js	0% <0%> (ø)
...src/commands/dao_cmds/acl_cmds/utils/knownRoles.js	0% <0%> (ø)	⬆️
.../aragon-cli/src/commands/dao_cmds/acl_cmds/view.js	0% <0%> (ø)	⬆️
packages/aragon-cli/src/lib/acl/viewFormatter.js	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d6e39c...3ee5fdd. Read the comment docs.

[dapplion]

@0xGabi In reference to all acl commands except acl view:
I would recommend considering the acl commands refactored. The next step would be to address the helpers and utils they depend on. However, after further review, there are too many dependencies of these utils, so it's not safe refactor them at the moment. It would be best to address all dao helpers/utils at once.

[0xGabi]

Made a small comments and suggest a little change to prevent an error when module is empty

[0xGabi]

@dapplion is this ACL structure sample complete?

[dapplion]

Until the PermissionsData level, I think so.

[0xGabi]

After doing other review I consider this is ready to be merged. There are a few cosmetic changes that we can discuss on next sync.

We discuss with Mathew to merge the PR first and then he'll take a look.