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

Upgrade to Arc v58, Rinkeby contract addresses #382

Merged
merged 26 commits into from
Dec 27, 2018
Merged

Upgrade to Arc v58, Rinkeby contract addresses #382

merged 26 commits into from
Dec 27, 2018

Conversation

dkent600
Copy link
Contributor

@dkent600 dkent600 commented Dec 6, 2018 •
edited
Loading

Resolves: #82

  • upgrades to Arc v58, including particularly changes to reputation bootstrapping contracts
  • adds contract addresses for rinkeby
  • Arc.js migration script now maintains the Ganache contract addresses supplied by the DAOstack migration repo, meaning that the account addresses have changed as a result.
  • ContributionReward.proposeContributionReward will now optionally take a proposal description that has already been hashed (and won't hash it). See ProposeContributionRewardParamsdescriptionIsHashed.
  • unstubs stubbed-out GenesisProtocol scheme parameters.
  • removes ContributionReward.ContributionProposal.contributionDescriptionHash

Breaking

  • Ganache account addresses have changed.
  • removes ContributionReward.ContributionProposal.contributionDescriptionHash

@dkent600 dkent600 self-assigned this Dec 6, 2018
tibetsprague
tibetsprague reviewed Dec 6, 2018
View reviewed changes
lib/wrappers/contributionReward.ts
@@ -154,7 +155,7 @@ export class ContributionRewardWrapper extends ProposalGeneratorBase {
eventContext,
this.contract.proposeContributionReward,
[options.avatar,
Utils.SHA3(options.description),
options.descriptionIsHashed ? options.description : Utils.SHA3(options.description),
Copy link
Contributor

Choose a reason for hiding this comment

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

makes a bit more sense to me to pass in whether you Arc.js to hash or not, so by default it would not be hashed, but this is ok too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tibetsprague I did it this way to avoid creating a breaking change. For what it is worth, it has the most convenient option as the default.

lib/wrappers/contributionReward.ts
@@ -587,7 +587,6 @@ export interface NewContributionProposalEventResult {
export interface ContributionProposal {
proposalId: Hash;
beneficiaryAddress: Address;
contributionDescriptionHash: Hash;
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this get removed?

Copy link
Contributor Author

@dkent600 dkent600 Dec 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@tibetsprague I forgot about that. I've added it to the list of breaking changes above. That property was removed from the contract as part of this PR: https://github.com/daostack/arc/pull/561/files

package.json Outdated
@@ -51,7 +51,7 @@
},
"homepage": "https://github.com/daostack/arc.js#readme",
"dependencies": {
"@daostack/migration": "0.0.0-alpha.56-v3",
"@daostack/migration": "0.0.0-alpha.58-v2",
Copy link
Contributor

Choose a reason for hiding this comment

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

backwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tibetsprague no, the earlier one is 56, the latter, 58

orenyodfat
orenyodfat reviewed Dec 9, 2018
View reviewed changes
config/default.json
@@ -18,6 +18,10 @@
"host": "127.0.0.1",
"port": 8546
},
"private": {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is private ? what the diff from ganache ?

Copy link
Contributor Author

@dkent600 dkent600 Dec 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

private is the name used for networks with an unknown id. ganache is a special case for a network running with a particular network id that arc.js supplies. So, for example, the user might have ganache running with a unknown network id, but arc.js can't know that it is in fact ganache, so will name it "private".

BTW, "private" is the name used by the migrations package and web3.

orenyodfat
orenyodfat reviewed Dec 27, 2018
View reviewed changes
package.json
@@ -63,7 +66,7 @@
"ethereumjs-abi": "^0.6.5",
"ethjs-abi": "0.1.8",
"fs-extra": "^5.0.0",
"ganache-cli": "^6.1.6",
"ganache-cli": "^6.2.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

6.2.5 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do in the Arc upgrade PR

@dkent600 dkent600 merged commit f52bf1d into master Dec 27, 2018
@dkent600 dkent600 deleted the upgradeArc branch December 27, 2018 15:35
@bohendo bohendo mentioned this pull request Jan 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tibetsprague tibetsprague tibetsprague left review comments

@orenyodfat orenyodfat orenyodfat approved these changes

@jellegerbrandy jellegerbrandy Awaiting requested review from jellegerbrandy

@ben-kaufman ben-kaufman Awaiting requested review from ben-kaufman

@tsuberim tsuberim Awaiting requested review from tsuberim

@leviadam leviadam Awaiting requested review from leviadam

Assignees

@dkent600 dkent600

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dkent600 @tibetsprague @orenyodfat