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

(Bug) Dutch- Publish page: uploaded file'Auth-os_addresses.txt' contains wrong names of contracts #1117

Merged
merged 4 commits into from
Sep 4, 2018
Merged

(Bug) Dutch- Publish page: uploaded file'Auth-os_addresses.txt' contains wrong names of contracts #1117

merged 4 commits into from
Sep 4, 2018

Conversation

mariano-aguero
Copy link
Contributor

Closes #1101

  • Check strategy of the crowdsale
  • Create labels regarding selected strategy

@mariano-aguero mariano-aguero added this to the 2.0 milestone Aug 30, 2018
@mariano-aguero mariano-aguero self-assigned this Aug 30, 2018
@dennis00010011b
Copy link

@mariano-aguero
For avoid to create one more PR it could be nice to make some additional work in this PR:
add to Auth-os_addresses.txt for Minted crowdsale two contract’s adresses:

  1. Sale_manager ( var REACT_APP_MINTED_CAPPED_CROWDSALE_MANAGER_ADDRESS)
  2. Token manager ( var REACT_APP_MINTED_CAPPED_TOKEN_MANAGER_ADDRESS)

@mariano-aguero
Copy link
Contributor Author

mariano-aguero commented Aug 30, 2018 •
edited
Loading

@dennis00010011b

Can you specific the ID of the issue ?

@coveralls
Copy link

coveralls commented Aug 30, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 3024

  • 0 of 13 (0.0%) changed or added relevant lines in 1 file are covered.
  • 98 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.1%) to 20.25%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/stepFour/utils.js 0 13 0.0%
Files with Coverage Reduction New Missed Lines %
src/components/stepFour/constants.js 1 0.0%
src/components/stepFour/utils.js 34 0.0%
src/components/stepFour/index.js 63 0.0%
Totals Coverage Status
Change from base Build 2976: -0.1%
Covered Lines: 834
Relevant Lines: 3610
💛 - Coveralls

@dennis00010011b
Copy link

@mariano-aguero
Sorry! #1118

@mariano-aguero
Copy link
Contributor Author

@dennis00010011b

For avoid to create one more PR it could be nice to make some additional work in this PR:
add to Auth-os_addresses.txt for Minted crowdsale two contract’s adresses:

Done

Closes #1118

@mariano-aguero mariano-aguero mentioned this pull request Aug 31, 2018
Closed
@dennis00010011b
Copy link

@mariano-aguero
Dutch-Publish page: app crashed while attempting to download file with metadata
Steps for reproducing: create Dutch crowdsale with any valid parameters and proceed until Publish page.
Issue not reproducible for Minted
screen shot 2018-09-02 at 5 43 33 am
dutch_download_crach.log

@mariano-aguero
Copy link
Contributor Author

mariano-aguero commented Sep 3, 2018 •
edited
Loading

@dennis00010011b

Dutch-Publish page: app crashed while attempting to download file with metadata ...

I added a validation.
This is because there is no environment variable "manager" for the selected strategy type (Dutch strategy, see the example env file)

vbaranov
vbaranov reviewed Sep 4, 2018
View reviewed changes
src/components/stepFour/utils.js Outdated
const labelIdx = isDutchStrategy ? 'DutchIdx' : 'MintedCappedIdx'
const labelCrowdsale = isDutchStrategy ? 'Dutch Crowdsale' : 'Sale'
const labelToken = isDutchStrategy ? 'Dutch Token' : 'Token'
const labelSaleManager = isDutchStrategy ? 'Dutch Crowdsale manager' : 'Sale manager'
Copy link
Collaborator

Choose a reason for hiding this comment

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

As Dutch strategy has no managers smart-contracts, here and in the next line, we could remove conditional operator and just leave values for Minted capped strategy

Copy link
Contributor Author

@mariano-aguero mariano-aguero Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@vbaranov Thanks for the feedback, change done

@vbaranov vbaranov self-requested a review September 4, 2018 12:05
@fernandomg fernandomg merged commit 889941d into 2.0 Sep 4, 2018
@ghost ghost removed the awaiting for review label Sep 4, 2018
@fernandomg fernandomg deleted the issue#1101 branch September 4, 2018 13:19
@kinexis-uk kinexis-uk mentioned this pull request Nov 5, 2022
Open
@cleancoindev cleancoindev mentioned this pull request Nov 5, 2022
Open
@larrycameron80 larrycameron80 mentioned this pull request Nov 5, 2022
Open
@kinexis-uk kinexis-uk mentioned this pull request Dec 25, 2022
Open
@cleancoindev cleancoindev mentioned this pull request Dec 25, 2022
Open
@larrycameron80 larrycameron80 mentioned this pull request Dec 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dennis00010011b dennis00010011b dennis00010011b approved these changes

@vbaranov vbaranov vbaranov approved these changes

@fernandomg fernandomg Awaiting requested review from fernandomg

Assignees

@mariano-aguero mariano-aguero

Labels
low priority migration to auth-os
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mariano-aguero @dennis00010011b @coveralls @vbaranov @fernandomg