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

Wrapper: new API for init() and initAccounts() #140

Merged
merged 12 commits into from Oct 19, 2018

Conversation

Projects
None yet
4 participants
@juslar
Copy link
Contributor

juslar commented Aug 28, 2018

Fix #128, implements API in #140 (comment).

@sohkai sohkai self-requested a review Aug 28, 2018

An optional array of accounts that the user controls
- `options` **[Object](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object)**
An optional options object that contains 'withAccounts' that specifies whether or not we should fetch accounts from the Web3 instance.
Ex: ```aragon.init(["0xbeefbeef"], { withAccounts: true })```

This comment has been minimized.

@Quazia

Quazia Aug 29, 2018

Collaborator

This example doesn't make sense with the way you have it set up. Currently, it will only pull accounts if the account param is null, it might make sense to have the accounts param and the web3 accounts combined if it's called in the way shown in this example.

This comment has been minimized.

@sohkai

sohkai Aug 29, 2018

Member

The two can't be combined into a single parameter because there may be times where we'd like to not provide an account list but still not fetch accounts from web3.

Given that users should know if they'd like to fetch accounts or provide their own by the time they init, a breaking API change would be to turn the entire argument list into a single options object with accounts and fetchAccountsFromConnectedWeb3 options. If both are provided, the web3 accounts could be concatenated with the provided accounts.

@sohkai sohkai self-assigned this Aug 30, 2018

@juslar

This comment has been minimized.

Copy link
Contributor

juslar commented Sep 4, 2018

Thanks! I made the change you requested @sohkai

juslar added some commits Sep 10, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Sep 14, 2018

@juslar Sorry for the delay... give us a bit to get back to this and review it :).

juslar and others added some commits Sep 17, 2018

@sohkai sohkai changed the title Wrapper: init() fetches accounts from web3 only if requested Wrapper: new API for init() and initAccounts() Oct 19, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Oct 19, 2018

@juslar I've updated this PR and modified it to the API in #140 (comment) now that we can break things with a new major version of aragon.js :).

I think the new API's a lot more clear, as it's more declarative, and also more easily lets us expand the options later if we need to in .init() :).

Thanks for helping get this started!

@izqui

izqui approved these changes Oct 19, 2018

Copy link
Member

izqui left a comment

LGTM!

* // Initialises the wrapper
* await aragon.init({
* accounts: {
* provided: ["0xbeefdead", "0xbeefbeef"]

This comment has been minimized.

@izqui

izqui Oct 19, 2018

Member
Suggested change Beta
* provided: ["0xbeefdead", "0xbeefbeef"]
* providedAccounts: ["0xbeefdead", "0xbeefbeef"]

@sohkai sohkai merged commit 9e40343 into aragon:master Oct 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment