Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

feat: throw semantic errors #109

Merged
merged 2 commits into from
Mar 22, 2018
Merged

feat: throw semantic errors #109

merged 2 commits into from
Mar 22, 2018

Conversation

epiqueras
Copy link
Contributor

@epiqueras epiqueras requested a review from satello March 21, 2018 23:31
@epiqueras epiqueras self-assigned this Mar 21, 2018
@epiqueras epiqueras added this to the March 19 -> March 26 milestone Mar 21, 2018
@coveralls
Copy link

coveralls commented Mar 21, 2018

Pull Request Test Coverage Report for Build 546

  • 120 of 209 (57.42%) changed or added relevant lines in 12 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 72.263%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/abstractWrappers/Notifications.js 3 4 75.0%
src/abstractWrappers/AbstractWrapper.js 0 2 0.0%
src/abstractWrappers/Disputes.js 12 14 85.71%
src/contractWrappers/CentralizedArbitratorWrapper.js 0 2 0.0%
src/contractWrappers/BlockHashRNGWrapper.js 0 3 0.0%
src/utils/StoreProviderWrapper.js 1 5 20.0%
src/contractWrappers/PinakionWrapper.js 9 13 69.23%
src/constants/error.js 29 34 85.29%
src/contractWrappers/ContractWrapper.js 8 14 57.14%
src/contractWrappers/KlerosWrapper.js 47 72 65.28%
Files with Coverage Reduction New Missed Lines %
src/abstractWrappers/Disputes.js 1 88.66%
Totals Coverage Status
Change from base Build 540: 0.08%
Covered Lines: 677
Relevant Lines: 861

💛 - Coveralls

satello
satello previously approved these changes Mar 22, 2018
Copy link
Contributor

@satello satello left a comment

Choose a reason for hiding this comment

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

Nice work! Good cleanup all around

} else {
throw new Error('unable to buy PNK')
}
await this._Arbitrator.buyPNK(amount, arbitratorAddress, account)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how clean these are now but we won't really get user friendly errors if we don't catch them since it will just throw either truffle contract or EVM errors (which I barely consider semantic). I am assuming you are planning on giving something more user friendly on the front end?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm i see you throw something nice a layer lower

@@ -261,6 +261,7 @@ describe('Dispute Resolution', () => {
klerosPOCAddress,
other
)
console.info(newState)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove log

openDisputes.push(disputeId)

// Advance to the next dispute
disputeId++
Copy link
Contributor

Choose a reason for hiding this comment

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

nice. much cleaner

@satello
Copy link
Contributor

satello commented Mar 22, 2018

Not sure what happened on that second test. Looks like a VM exception slipped through the cracks somewhere

@epiqueras
Copy link
Contributor Author

@satello
It was a notification test timeout, retrying.

@satello satello merged commit 3fbe8b3 into develop Mar 22, 2018
@epiqueras epiqueras deleted the feat/throw-semantic-errors branch March 22, 2018 03:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants