Fix transaction related dependency - Closes #3614 #3620

shuse2 · 2019-05-14T20:37:06Z

What was the problem?

Transaction related logic had dependency everywhere and logic was scattered.

How did I fix it?

Transaction pool depends on blocks
blocks still temporally depends on transaction pool
removed proxy transaction.js and move API related logic to node status (only used for node status endpoint)
Removed dependency of blocks (_getCurrentContext) from process_transactions
Changed process transactions to function
Directly expose transaction pool as module
Skipped/replaced dapp related/using tests
Removed all callbacks in transactions and transaction pool

How to test it?

Run the application
Check the changed tests carefully
Run the tests

Review checklist

The PR resolves Combine all the transaction related logic into one module and expose used functionality #3614
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

… 3614-fix_transaction_related_dependency

framework/src/modules/chain/loader.js

framework/src/modules/chain/transactions/votes.js

SargeKhan

I think there is a lack of consistency in the usage of transactions module in the application. In some cases we require the transaction module or some of it fuctions whereas in other cases, we use scope to initialize the module variable.
It leaves the responsibility to identify which parts of the module need to be imported and which parts need to be passed via scope to the coder. And this will lead to difficulty in understanding and inconsistency in one clearly defined approach to using modules.

framework/src/modules/http_api/controllers/node.js

framework/src/modules/chain/init_steps/init_logic_structs.js

framework/src/modules/chain/submodules/blocks/chain.js

lsilvs · 2019-05-23T09:32:45Z

framework/src/modules/chain/chain.js

@@ -319,6 +307,53 @@ module.exports = class Chain {
 		this.logger.info('Cleaned up successfully');
 	}

+	async _initModules() {


Can't we move all this logic to ./init_steps/init_modules?

we can move to the init_modules file, but it will not be dynamic anymore.
so probably it's easier to access all the required parameters here

shuse2 · 2019-05-23T16:24:20Z

I have separated transaction_manager to different folder and renamed it to interface_adapters/transaction_interface_adapter.js after discussion with @SargeKhan

SargeKhan · 2019-05-24T08:10:33Z

framework/src/modules/chain/submodules/blocks/chain.js

 			},
 			channel,
 		};
 		self = this;
+		self.modules = {


Can we assign interfaceAdapters to a different variable name. Lest we will have self.modules and module variables which have different values.

true, but this will be changed in the coming block PR

SargeKhan · 2019-05-24T08:15:47Z

framework/src/modules/chain/transport.js

@@ -77,7 +77,7 @@ class Transport {
 			scope.config.nonce,
 			scope.config.broadcasts,
 			scope.config.forging.force,
-			scope.logic.transactionPool,
+			scope.modules.transactionPool,


Why do we bind transactionPool in the onBind function this file already receives it in the constructor?

This is because I didn't want to touch the bind method (since it will be removed), and it will break the test ='(
forgers, transport and loader needs to be refactored after the block PR

… 3614-fix_transaction_related_dependency

shuse2 and others added 2 commits May 14, 2019 18:59

Add initial structure

fbb60a0

Cleanup sample

eec5965

shuse2 added the 🏗️ in progress label May 14, 2019

shuse2 changed the base branch from 3580_refactor_entry_modules to development May 15, 2019 04:15

MaciejBaj assigned shuse2 May 15, 2019

shuse2 assigned pablitovicente May 15, 2019

pablitovicente mentioned this pull request May 16, 2019

Name of unsigned transactions queue is misleading #3610

Closed

shuse2 force-pushed the 3614-fix_transaction_related_dependency branch from 582ff25 to c38f237 Compare May 18, 2019 19:45

shuse2 added 15 commits May 18, 2019 21:47

♻️ Remove event emitter

0173e31

♻️ Change exception position

73e5d60

♻️ Clean up unused function

bfe4aa9

Remove globals

729d0ab

🔥 Remove globals

b7ec384

♻️ Add temo block_slots

c20aaeb

Update reference

54a6e2f

🔥 Remove duplicate files

6c4268c

♻️ Complete moving transactions related files

5c7ba4d

Move confirmed transactions to API logic

ba3998a

✅ Move unit tests and fix

edad280

Fix common application test setup and unit tests

57023b3

Fix block process and verify unit test

5e3929a

Fix loader unit test

1fca674

Fix node status unit test

7425530

shuse2 force-pushed the 3614-fix_transaction_related_dependency branch from c38f237 to 7425530 Compare May 18, 2019 19:47

shuse2 unassigned pablitovicente May 18, 2019

shuse2 added 5 commits May 18, 2019 22:37

♻️ Split transaction pool and change logic to be more functional

175196e

Fix unit test for transactions and transaction pool

7db3504

Fix unit tests

9d23ab9

Fix context reading and test file name

9806080

Fix chain test

35beebe

✅ Fix node unit test

a1a0474

shuse2 requested review from SargeKhan, pablitovicente and lsilvs May 19, 2019 17:10

shuse2 added 👁️ pending review and removed 🏗️ in progress labels May 19, 2019

shuse2 marked this pull request as ready for review May 19, 2019 17:10

pablitovicente approved these changes May 21, 2019

View reviewed changes

Merge branch 'development' of https://github.com/LiskHQ/lisk-sdk into…

8823bec

… 3614-fix_transaction_related_dependency

SargeKhan suggested changes May 22, 2019

View reviewed changes

framework/src/modules/chain/loader.js Outdated Show resolved Hide resolved

framework/src/modules/chain/loader.js Show resolved Hide resolved

framework/src/modules/chain/transactions/votes.js Outdated Show resolved Hide resolved

shuse2 and others added 3 commits May 22, 2019 17:44

♻️ Remove duplicate check

6dea688

♻️ Accept empty object for exceptions

3ace361

Handle non existence exception key correctly

e38b83d

shuse2 requested a review from SargeKhan May 23, 2019 06:54

SargeKhan suggested changes May 23, 2019

View reviewed changes

lsilvs reviewed May 23, 2019

View reviewed changes

shuse2 added 4 commits May 23, 2019 16:40

Update the folder structure for interface adapters

935ad8f

Make node endpotion execution parallel

2c84c79

🐛 Fix naming of interface adapters

a63dd85

🐛 Fix wrong stub

f9cc4a8

♻️ Fix wrong file name

1e30180

SargeKhan reviewed May 24, 2019

View reviewed changes

shuse2 requested a review from SargeKhan May 24, 2019 08:49

SargeKhan approved these changes May 24, 2019

View reviewed changes

Merge branch 'development' of https://github.com/LiskHQ/lisk-sdk into…

37fda17

… 3614-fix_transaction_related_dependency

shuse2 added ✅ ready and removed 👁️ pending review labels May 24, 2019

shuse2 merged commit 6899965 into development May 24, 2019

shuse2 deleted the 3614-fix_transaction_related_dependency branch June 8, 2019 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix transaction related dependency - Closes #3614 #3620

Fix transaction related dependency - Closes #3614 #3620

shuse2 commented May 14, 2019 •

edited

Loading

SargeKhan left a comment

lsilvs May 23, 2019

shuse2 May 23, 2019

shuse2 commented May 23, 2019

SargeKhan May 24, 2019

shuse2 May 24, 2019

SargeKhan May 24, 2019

shuse2 May 24, 2019

Fix transaction related dependency - Closes #3614 #3620

Fix transaction related dependency - Closes #3614 #3620

Conversation

shuse2 commented May 14, 2019 • edited Loading

What was the problem?

How did I fix it?

How to test it?

Review checklist

SargeKhan left a comment

Choose a reason for hiding this comment

lsilvs May 23, 2019

Choose a reason for hiding this comment

shuse2 May 23, 2019

Choose a reason for hiding this comment

shuse2 commented May 23, 2019

SargeKhan May 24, 2019

Choose a reason for hiding this comment

shuse2 May 24, 2019

Choose a reason for hiding this comment

SargeKhan May 24, 2019

Choose a reason for hiding this comment

shuse2 May 24, 2019

Choose a reason for hiding this comment

shuse2 commented May 14, 2019 •

edited

Loading