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.

Sign up for GitHub

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

Jump to bottom

getTransactionPath refactor #840

Merged
merged 18 commits into from
Oct 31, 2019
Merged

getTransactionPath refactor #840

merged 18 commits into from
Oct 31, 2019

Conversation

macor161
Copy link
Contributor

@macor161 macor161 commented Oct 26, 2019 •
edited
Loading

🦅 Pull Request

  • Fix comments from Update wrapper to rc.17 #796 (comment)
  • Move getTransactionPath logic to a single place
  • Replace onError with thrown exceptions in initAragonJS
  • Move aragonjs-wrapper to helpers folder as we discussed with @ajsantander and @0xGabi that it would be better to have a central place for all utils. Let me know if you had another place in mind.

Once #837 is merged, I will also remove my calls to the hardcoded ABIs.

🚨 Test instructions

dao exec, dao acl and dao apps commands.

✔️ 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

@macor161 macor161 requested a review from 0xGabi as a code owner October 26, 2019 16:51
@eternauta1337 eternauta1337 mentioned this pull request Oct 29, 2019
Closed
6 tasks
0xGabi
0xGabi approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Great PR! 💪
Everything looks much clear and maintainable.

Note: A small detail is that the Kernel appName is not showing when using dao acl. It shows the proxy address instead.

@0xGabi 0xGabi merged commit 1c816f6 into aragon:master Oct 31, 2019
0xGabi pushed a commit that referenced this pull request Oct 31, 2019
@macor161
getTransactionPath refactor (#840)
ef9f96f
@macor161
Copy link
Contributor Author

Thanks @0xGabi I'm going to look into that.

@macor161 macor161 deleted the gettransactionpath-refactor branch October 31, 2019 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xGabi 0xGabi 0xGabi approved these changes

@kernelwhisperer kernelwhisperer Awaiting requested review from kernelwhisperer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@macor161 @0xGabi