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

ARA-667 'dao install' #1848

Merged
merged 4 commits into from
Mar 31, 2021
Merged

ARA-667 'dao install' #1848

merged 4 commits into from
Mar 31, 2021

Conversation

c0rv0s
Copy link
Contributor

@c0rv0s c0rv0s commented Mar 30, 2021

🦅 Pull Request Description

Original issue: #1764

In the bug report the user describes an issue with installing an app and having it not show up in the DAO UI. However they didn't realize that the app installation process had to be completed with a vote.

The changes here will check if a vote is required to finish installation of an app and inform the user once the install command finishes executing.

A follow up issue ARA-822 has been added to add a test case for this.

Rational

The app install process is functioning as intended but the communication to the user of the next steps required was insufficient.

🚨 Test instructions

Install a new app into a dao where the Voting app has the APP_MANAGER permission and at the end of the install command there will be a message saying to check the Voting app for a new vote to complete the installation.

@c0rv0s c0rv0s requested review from cbrzn and nestorbe March 30, 2021 15:04
@c0rv0s c0rv0s self-assigned this Mar 30, 2021
@linear
Copy link

linear bot commented Mar 30, 2021

ARA-667 'dao install' sent a transaction which doesn't install an app

🐛 Bug Report

After executing 'dao install' the tool has executed a transaction which doesn't end up with the app being installed

Have you read the Contributing Guidelines on issues?

  • Yes

To Reproduce

aragon dao install stablefund.aragonid.eth token-request.aragonpm.eth --app-init-args 0xe8aa182d10252bed21cbe35a1f8cac9b0d9458a7 0xc1ee97ce717c1dfed21543ca56f23303c1d5dffc "[]" --environment aragon:mainnet --gas-price 37
✔ Fetching token-request.aragonpm.eth@latest
✔ Fetching token-request.aragonpm.eth@latest
↓ Checking installed version [skipped]
→ Installing the first instance of token-request.aragonpm.eth in DAO
✔ Deploying app instance
↓ Fetching deployed app [skipped]
→ App wasn't deployed in transaction.

ℹ Successfully executed: "Execute desired action as a token holder"

TX:
https://etherscan.io/tx/0x11911c9abc21ab2f9545088276fcfbca6a237a3328a377e335d3ddc105194092

App is not seen in the configuration, on UI or 'aragon dao apps'

Expected behavior

I expect the cli to install the new app which can be seen and configured

Actual Behavior

TX created:
https://etherscan.io/tx/0x11911c9abc21ab2f9545088276fcfbca6a237a3328a377e335d3ddc105194092

app not seen anywhere in the configuration

Context

mainnet

Organization

stablefund.aragonid.eth

Environment

MacOS Mojave
CLI from NPM

View original issue in GitHub


0xGabi 2020-07-14

Hi @ethene this an expected behavior. If you look into the success message: Execute desired action as a token holder what the aragonCLI did was trying to install the app but as you don't have the full permissions to do it directly it creates a Vote that if gets approve the app will be installed. Looking into your organization, this is the open vote: https://mainnet.aragon.org/#/stablefund/0x241b27133d78177f1fb37ea26645bff0d1e1bbad/vote/0/

If you are the only owner fo the organization and you would like to do a quick setup without having to vote on every change, you can assign your account the role to create permissions: https://mainnet.aragon.org/#/stablefund/permissions/app/0xe0f35ac8e6c8b8ab88d4798becc0865496d623ba

The default configuration of most Aragon Organization Templates has a Voting app as the most privileged.

ethene 2020-07-14

Thanks for the explanation.
Now that the vote has been approved what next steps needs to be performed to install the app?
I still don't see it in the configuration.

sohkai 2020-07-14

@ethene You just need to assign the app a permission!

ethene 2020-07-14

@sohkai app is not seen in the configuration yet, it is not possible to assign any permissions to it.

I approved a vote per @0xGabi suggestion, the app did not appear.

Then I added my account a role to create permissions per this suggestion too.

After executing the 'install' command again I see the following:

aragon dao install stablefund.aragonid.eth token-request.aragonpm.eth --app-init-args 0xe8aa182d10252bed21cbe35a1f8cac9b0d9458a7 0xc1ee97ce717c1dfed21543ca56f23303c1d5dffc "[]" --environment aragon:mainnet --gas-price 33
✔ Fetching token-request.aragonpm.eth@latest
✔ Fetching token-request.aragonpm.eth@latest
✔ Checking installed version
✔ Deploying app instance
↓ Fetching deployed app [skipped]
→ App wasn't deployed in transaction.

ℹ Successfully executed: "Execute desired action as a token holder"

I see another vote created and the app still does not exist in the configuration.
Really disappointed there is no proper documentation on the app installations at all

sohkai 2020-07-14

Thanks for the feedback @ethene, we are equally as disappointed in ourselves.

Your app is installed—it's just a bit of an awkward path to find its address so you can assign a permission to it as you weren't able to complete the interaction through the CLI.

0xc0af05fdf7e8338ee5359b43a1c69a3261153ba6 is your new app's address!

ethene 2020-07-14

What can I do with the address to link it to the DAO app center to enable it? I noticed there is no choice in the UI and what cli commands it do I need to perform for it?

Copy link
Contributor

@nestorbe nestorbe left a comment

Choose a reason for hiding this comment

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

LGTM!

@c0rv0s c0rv0s merged commit ff99add into master Mar 31, 2021
@c0rv0s c0rv0s deleted the ARA-667-dao-install-msg branch March 31, 2021 15:07
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.

None yet

3 participants