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

Add functionality for handling exceptions present in the blockchain - Closes #3181 #3195

Conversation

SargeKhan
Copy link
Contributor

What was the problem?

The exceptions in the testnet and mainnet were not handled in the new implementation of transaction processing.

How did I fix it?

Added exception handling logic for various type of exceptions defined in testnet.

How to test it?

Run integration tests for transactions/exceptions/

Review checklist

@SargeKhan SargeKhan changed the base branch from development to feature/improve_transactions_processing_efficiency March 26, 2019 18:27
Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, just minor issues!
Also, GJ adding integration tests for exceptions =)

@@ -31,7 +31,7 @@ describe('system test (type 3) - voting with duplicate submissions', () => {
let t = 0;

/* eslint-disable no-loop-func */
while (i < 30) {
while (i < 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think i've seen this before too, but probably want to put it back to 30?

framework/test/mocha/integration/common.js Show resolved Hide resolved
@@ -0,0 +1,200 @@
const expect = require('chai').expect;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const expect = require('chai').expect;
const { expect } = require('chai');

Also, header required

@@ -0,0 +1,272 @@
const expect = require('chai').expect;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const expect = require('chai').expect;
const { expect } = require('chai');

Also, header required


const exceptions = global.exceptions;

describe('exceptions for multisingature transactions', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

multisignature

Copy link
Contributor

@pablitovicente pablitovicente left a comment

Choose a reason for hiding this comment

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

Looks good in general

  • Need to merge feature branch into this one
  • Tests are using same DB so they do not run in parallel so when calling localCommon.beforeBlock( make sure to use different strings for the type parameter for this function

Copy link
Contributor

@mitsuaki-u mitsuaki-u left a comment

Choose a reason for hiding this comment

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

LGTM, only one concern regarding ordering of errors

@SargeKhan SargeKhan requested review from pablitovicente, shuse2 and mitsuaki-u and removed request for pablitovicente and shuse2 April 23, 2019 08:34
@SargeKhan SargeKhan merged commit e41743b into feature/improve_transactions_processing_efficiency Apr 23, 2019
@shuse2 shuse2 deleted the 3181-handle_exceptions branch April 23, 2019 13:50
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.

5 participants