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

Consistent dependencies on transaction types - Closes #2779 #2844

Merged

Conversation

2snEM6
Copy link
Contributor

@2snEM6 2snEM6 commented Feb 7, 2019

What was the problem?

Transactions logic didn't have a consistent common interface. Some of the logic presented completely different constructor definitions even passing the same dependencies.

How did I fix it?

Unifying (to some extent) how Transactions logic is instantiated and simplified their constructor definition.

How to test it?

Run unit tests

Review checklist

@2snEM6 2snEM6 self-assigned this Feb 7, 2019
@MaciejBaj MaciejBaj changed the title 2779 consistent dependencies on transaction types Consistent dependencies on transaction types - Closes #2779 Feb 7, 2019
@2snEM6 2snEM6 changed the base branch from 2762-lisk-framework-structure to development February 8, 2019 08:53
@2snEM6
Copy link
Contributor Author

2snEM6 commented Feb 12, 2019

@nazarhussain ready for review whenever you can

Copy link

@diego-G diego-G left a comment

Choose a reason for hiding this comment

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

Clean out unit test from done() when possible and use async instead of return.

framework/src/modules/chain/logic/in_transfer.js Outdated Show resolved Hide resolved
framework/src/modules/chain/logic/vote.js Outdated Show resolved Hide resolved
framework/src/modules/chain/logic/delegate.js Outdated Show resolved Hide resolved
framework/src/modules/chain/logic/dapp.js Outdated Show resolved Hide resolved
framework/src/modules/chain/logic/dapp.js Outdated Show resolved Hide resolved
@2snEM6 2snEM6 force-pushed the 2779_consistent_dependencies_on_transaction_types branch 5 times, most recently from f6f7e7e to 47695b0 Compare February 18, 2019 13:02
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

@limiaspasdaniel Just one last request, if we use __scope compared to __private that will link up the concept of scope on higher level. The __ in start already specifying convention that its private.

framework/src/modules/chain/logic/in_transfer.js Outdated Show resolved Hide resolved
@2snEM6 2snEM6 force-pushed the 2779_consistent_dependencies_on_transaction_types branch 3 times, most recently from 353b351 to 5c24266 Compare February 19, 2019 09:46
nazarhussain
nazarhussain previously approved these changes Feb 19, 2019
framework/src/modules/chain/logic/dapp.js Show resolved Hide resolved
framework/src/modules/chain/logic/delegate.js Outdated Show resolved Hide resolved
framework/src/modules/chain/logic/multisignature.js Outdated Show resolved Hide resolved
framework/src/modules/chain/logic/multisignature.js Outdated Show resolved Hide resolved
Copy link

@diego-G diego-G left a comment

Choose a reason for hiding this comment

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

I would continue a bit further extending the issue to cover the root of all transaction types. https://github.com/LiskHQ/lisk/blob/5c242660e88aaef3e536770d883de22c71f4d2d3/framework/src/modules/chain/logic/transaction.js#L50
What do you think @limiaspasdaniel ? Open a new issue if you consider it appropriate.

@2snEM6
Copy link
Contributor Author

2snEM6 commented Feb 19, 2019

I totally agree @diego-G , it will definitely look cleaner and it makes sense. I will open another issue so this PR doesn't get too big.

@2snEM6 2snEM6 force-pushed the 2779_consistent_dependencies_on_transaction_types branch from ed01c2e to db8e37f Compare February 20, 2019 13:55
@2snEM6 2snEM6 force-pushed the 2779_consistent_dependencies_on_transaction_types branch from bbf6a4d to 2bb21f4 Compare February 20, 2019 14:39
@MaciejBaj MaciejBaj merged commit b4bc9b8 into 2866-chain-init Feb 20, 2019
@MaciejBaj MaciejBaj deleted the 2779_consistent_dependencies_on_transaction_types branch February 20, 2019 17:38
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.

None yet

4 participants