kredits/contracts
9
1
Fork 0
Code Issues 21 Pull Requests Actions Projects 1 Releases 8 Activity

WIP: Fix Aragon breaking changes; update dependencies #182

Merged
raucao merged 9 commits from chore/update_dependencies into master 2020-02-20 11:42:04 +00:00
Conversation 16 Commits 9 Files Changed 15
+3202 -96354
raucao commented 2020-02-08 21:29:52 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Dance the update dance with me.

arapp.json exception

The only documented options for the apm config property are now related to IPFS. So I removed what seem to be outdated values on our end, and the devchain works again.

Other Aragon CLI changes

  • The Aragon CLI package was still a dependency in every app's package.json. I removed it everywhere.
  • There is no aragon contracts anymore. Neither compile nor test. It looks to me like truffle test should work (it runs stuff and complains about things), but there are test failures now. Probably due to other oudated API calls in the tests (e.g. dao.acl()).

App package questions

@haythem96, @bumi: To be honest, the separate package.json files look like a mess to me, and I don't understand how these are used exactly, because the Travis config is not actually running npm install-all before running tests. Travis also doesn't seem to compile contracts before testing, so unless you commit and push compiled changes, I don't see how it could catch test failures in PRs.

These question should be solved either in this PR or with a new one focused on fixing the test setup. I won't touch the code in this branch from here on, so please feel free to work on this branch and push fixed tests.

closes #179

Dance the update dance with me. ### arapp.json exception The only [documented options for the `apm` config property](https://hack.aragon.org/docs/cli-global-confg#the-arappjson-file) are now related to IPFS. So I removed what seem to be outdated values on our end, and the devchain works again. ### Other Aragon CLI changes * The Aragon CLI package was still a dependency in every app's `package.json`. I removed it everywhere. * There is no `aragon contracts` anymore. Neither `compile` nor `test`. It looks to me like `truffle test` should work (it runs stuff and complains about things), but there are test failures now. Probably due to other oudated API calls in the tests (e.g. `dao.acl()`). ### App package questions @haythem96, @bumi: To be honest, the separate `package.json` files look like a mess to me, and I don't understand how these are used exactly, because the Travis config is not actually running `npm install-all` before running tests. Travis also doesn't seem to compile contracts before testing, so unless you commit and push compiled changes, I don't see how it could catch test failures in PRs. These question should be solved either in this PR or with a new one focused on fixing the test setup. I won't touch the code in this branch from here on, so please feel free to work on this branch and push fixed tests. closes #179
bumi commented 2020-02-10 13:47:38 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Thanks for this PR! I could get the contracts running using this update. (pushed a minor update here)

I am not sure about travis and the tests currently fail on my machine:

$ npm run test

1) Contract: Token app
       "before all" hook:
     TypeError: dao.acl is not a function
      at Context.before (test/token.js:25:43)
Thanks for this PR! I could get the contracts running using this update. (pushed a minor update here) I am not sure about travis and the tests currently fail on my machine: ``` $ npm run test 1) Contract: Token app "before all" hook: TypeError: dao.acl is not a function at Context.before (test/token.js:25:43) ```
raucao (Migrated from github.com) reviewed 2020-02-10 13:58:51 +00:00
scripts/deploy-kit.js
raucao (Migrated from github.com) commented 2020-02-10 13:58:51 +00:00
Copy Link
Copy Source

arapp.environments[environment].apm has to be an object now, so the first condition here is not going to work anymore.

`arapp.environments[environment].apm` has to be an object now, so the first condition here is not going to work anymore.
bumi (Migrated from github.com) reviewed 2020-02-10 14:02:28 +00:00
scripts/deploy-kit.js
bumi (Migrated from github.com) commented 2020-02-10 14:02:28 +00:00
Copy Link
Copy Source

what does not work anymore?

that apm entry in the arapp.json (removed in this PR) was added/used by us - and not aragon related.
We had used that entry when open.aragonpm.eth was not available on all networks (and we had to use aragonpm.eth locally.

now we can default to open.aragonpm.eth

what does not work anymore? that `apm` entry in the arapp.json (removed in this PR) was added/used by us - and not aragon related. We had used that entry when `open.aragonpm.eth` was not available on all networks (and we had to use `aragonpm.eth` locally. now we can default to `open.aragonpm.eth`
bumi commented 2020-02-10 14:02:57 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I've updated some tests, now it fails with:

  1) Contract: Contributor app
       "before all" hook:
     Error: invalid bytes value (arg="_initializePayload", coderType="bytes", value=0)
     at PromiEvent (/home/bumi/.node_modules/lib/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js:9:1)
      at TruffleContract.newAppInstance (/home/bumi/.node_modules/lib/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js:169:1)
      at Context.before (test/contributor.js:37:31)
      at process._tickCallback (internal/process/next_tick.js:68:7)
I've updated some tests, now it fails with: ``` 1) Contract: Contributor app "before all" hook: Error: invalid bytes value (arg="_initializePayload", coderType="bytes", value=0) at PromiEvent (/home/bumi/.node_modules/lib/node_modules/truffle/build/webpack:/packages/contract/lib/promievent.js:9:1) at TruffleContract.newAppInstance (/home/bumi/.node_modules/lib/node_modules/truffle/build/webpack:/packages/contract/lib/execute.js:169:1) at Context.before (test/contributor.js:37:31) at process._tickCallback (internal/process/next_tick.js:68:7) ```
raucao (Migrated from github.com) reviewed 2020-02-10 14:04:04 +00:00
scripts/deploy-kit.js
raucao (Migrated from github.com) commented 2020-02-10 14:04:04 +00:00
Copy Link
Copy Source

Please read my original PR description and follow the link. The entire reason for opening this PR was an exception about this particular arapp.json property having to be an object now.

Please read my original PR description and follow the link. The entire reason for opening this PR was an exception about this particular `arapp.json` property having to be an object now.
raucao (Migrated from github.com) reviewed 2020-02-10 14:04:55 +00:00
scripts/deploy-kit.js
raucao (Migrated from github.com) commented 2020-02-10 14:04:54 +00:00
Copy Link
Copy Source

that apm entry in the arapp.json (removed in this PR) was added/used by us - and not aragon related.

That would explain why it's not documented. They introduced this property themselves in the meantime, but with different keys.

> that `apm` entry in the arapp.json (removed in this PR) was added/used by us - and not aragon related. That would explain why it's not documented. They introduced this property themselves in the meantime, but with different keys.
bumi (Migrated from github.com) reviewed 2020-02-10 14:15:00 +00:00
scripts/deploy-kit.js
bumi (Migrated from github.com) commented 2020-02-10 14:14:59 +00:00
Copy Link
Copy Source

this is a WIP PR, isn't it?
I was trying to debug that issue and get this update running here.

that configuration conflict is not solved, yet and the tests do not run, yet.

re: apm entry in the arapp.json file I tend to not have the apm registry configurable anymore. We now can rely on open.aragonpm.eth (locally and for all Ethereum netwroks we want to deploy to)

this is a WIP PR, isn't it? I was trying to debug that issue and get this update running here. that configuration conflict is not solved, yet and the tests do not run, yet. re: `apm` entry in the arapp.json file I tend to not have the apm registry configurable anymore. We now can rely on `open.aragonpm.eth` (locally and for all Ethereum netwroks we want to deploy to)
raucao (Migrated from github.com) reviewed 2020-02-10 14:22:22 +00:00
scripts/deploy-kit.js
raucao (Migrated from github.com) commented 2020-02-10 14:22:22 +00:00
Copy Link
Copy Source

Not sure how your comment relates to what I'm trying to explain. The config issue was solved by me removing the custom apm property, because Aragon now requires it to be their own documented object as a value (the one with the IPFS configs). The test failures are a different topic.

I explained these things in the original PR description, with separate headlines and paragraphs for the various topics this PR touches.

Not sure how your comment relates to what I'm trying to explain. The config issue was solved by me removing the custom `apm` property, because Aragon now requires it to be their own documented object as a value (the one with the IPFS configs). The test failures are a different topic. I explained these things in the original PR description, with separate headlines and paragraphs for the various topics this PR touches.
raucao (Migrated from github.com) reviewed 2020-02-10 14:25:00 +00:00
scripts/deploy-kit.js
raucao (Migrated from github.com) commented 2020-02-10 14:25:00 +00:00
Copy Link
Copy Source

To summarize my one comment on this one line of code again:

arapp.environments[environment].apm cannot be a string with an APM address anymore, as per their documentation. And it fails with an exception when trying to execute any aragon-cli command that encounters this config property as a string.

I hadn't seen that it was used in other places, so what fixed the aragon commands for me was just to remove the keys from arapp.json.

To summarize my one comment on this one line of code again: `arapp.environments[environment].apm` cannot be a string with an APM address anymore, as per their documentation. And it fails with an exception when trying to execute any aragon-cli command that encounters this config property as a string. I hadn't seen that it was used in other places, so what fixed the aragon commands for me was just to remove the keys from `arapp.json`.
bumi (Migrated from github.com) reviewed 2020-02-10 15:03:57 +00:00
scripts/deploy-kit.js
bumi (Migrated from github.com) commented 2020-02-10 15:03:57 +00:00
Copy Link
Copy Source

which broke the setup/deployment of kredits and that's what I am trying to fix here...
It seems you did not change any code there?!

And I know that apm is now a aragon related property and can not be used and I also read your comments...

maybe I should better make a PR to your PR then we don't need to somewhat discuss WIPs and avoid missunderstandings.

which broke the setup/deployment of kredits and that's what I am trying to fix here... It seems you did not change any code there?! And I know that `apm` is now a aragon related property and can not be used and I also read your comments... maybe I should better make a PR to your PR then we don't need to somewhat discuss WIPs and avoid missunderstandings.
bumi commented 2020-02-10 15:06:21 +00:00 (Migrated from github.com)
Copy Link
Copy Source

ToDo:

ToDo: - [ ] Update daoFactory address to use v0.8 of aragonOS. This should be the one deployed by the updated aragonOS package see: https://github.com/aragon/deployments/tree/master/environments/rinkeby
raucao (Migrated from github.com) reviewed 2020-02-10 15:09:05 +00:00
scripts/deploy-kit.js
raucao (Migrated from github.com) commented 2020-02-10 15:09:04 +00:00
Copy Link
Copy Source

I cannot know what you know, because I don't have a telepathic link to you. Until this last comment it seemed like you hadn't understood my original comment. And my original comment was merely a pointer to that config option being outdated.

If you already understand something that someone points out, then why not just say "Yeah, I know, I'm working on that"? Or even nothing at all, if it's of no consequence to my involvement.

I cannot know what you know, because I don't have a telepathic link to you. Until this last comment it seemed like you hadn't understood my original comment. And my original comment was *merely a pointer* to that config option being outdated. If you already understand something that someone points out, then why not just say "Yeah, I know, I'm working on that"? Or even nothing at all, if it's of no consequence to my involvement.
bumi (Migrated from github.com) reviewed 2020-02-10 15:15:31 +00:00
scripts/deploy-kit.js
bumi (Migrated from github.com) commented 2020-02-10 15:15:31 +00:00
Copy Link
Copy Source

to me it seems you did not understand what that apm was used for and why your change broke our deploy script.
but I also don't know what you know, because I also don't have a telepathic link to you.

anyhow this discussion now seems to be unrelated to this PR and also on already outdated code, so I am resolving this now.

to me it seems you did not understand what that `apm` was used for and why your change broke our deploy script. but I also don't know what you know, because I also don't have a telepathic link to you. anyhow this discussion now seems to be unrelated to this PR and also on already outdated code, so I am resolving this now.
bumi commented 2020-02-20 11:40:19 +00:00 (Migrated from github.com)
Copy Link
Copy Source

merging this as discussed. The other fixes should be done in separate PRs.
keeping the WIP to indicate some work is missing

merging this as discussed. The other fixes should be done in separate PRs. keeping the WIP to indicate some work is missing
bumi commented 2020-02-20 11:40:36 +00:00 (Migrated from github.com)
Copy Link
Copy Source

btw. @haythem96 can you help with the contract upgrades?

btw. @haythem96 can you help with the contract upgrades?
bumi (Migrated from github.com) approved these changes 2020-02-20 11:41:05 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
raucao
bumi
Labels
Clear labels
good first issue
Good for newcomers
 ipfs
rsk
scaling
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/contracts#182