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.

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

Remove old vote transaction and exception handling - Closes #5074 #5082

Merged
merged 1 commit into from Apr 2, 2020

Conversation

shuse2
Copy link
Member

@shuse2 shuse2 commented Apr 1, 2020

What was the problem?

This PR resolves #5074

How was it solved?

  • Remove old vote transaction and handling of it
  • Remove exception handling

How was it tested?

  • All test passes
  • Application runs as expected

@shuse2 shuse2 self-assigned this Apr 1, 2020
@shuse2 shuse2 force-pushed the 5074-remove_old_vote branch 3 times, most recently from d892389 to 89c88c6 Compare April 1, 2020 16:00
@shuse2 shuse2 marked this pull request as ready for review April 1, 2020 16:12
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.

Can we make the transaction numbers sequential?

this PR leaves:

8: TransferTransaction,	
10: DelegateTransaction,
12: MultisignatureTransaction,
13: VoteTransaction,
14: UnlockTransaction,

better to leave it as:

9: TransferTransaction,	
10: DelegateTransaction,
11: VoteTransaction,
12: MultisignatureTransaction,
13: UnlockTransaction,

(It seems when we deleted the 2nd signature we were already jumping one number but I think it would be much nicer if numbers are sequential)

The rest looks good. Thoughts?

@shuse2
Copy link
Member Author

shuse2 commented Apr 2, 2020

if we proceed with https://research.lisk.io/t/introduce-numbering-scheme-for-transaction-types/220, it would be completely different numbering, too
I would wait for the idea to go into the LIP repo to change it
Also, i just realized, but maybe it's bit better if vote and unlock are next to each other coz they are related

@pablitovicente
Copy link
Contributor

if we proceed with https://research.lisk.io/t/introduce-numbering-scheme-for-transaction-types/220, it would be completely different numbering, too
I would wait for the idea to go into the LIP repo to change it
Also, i just realized, but maybe it's bit better if vote and unlock are next to each other coz they are related

OK. Then maybe create the issue for the renumbering so it's in the backlog?

@pablitovicente pablitovicente self-requested a review April 2, 2020 07:17
Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +30 to -36
export const validateTransactions = () => (
transactions: ReadonlyArray<BaseTransaction>,
): ReadonlyArray<TransactionResponse> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we no longer have exceptions.. why don't we change function to??

export const validateTransactions = (transactions: ReadonlyArray<BaseTransaction>): ReadonlyArray<TransactionResponse> => 

Copy link
Member Author

@shuse2 shuse2 Apr 2, 2020

Choose a reason for hiding this comment

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

I think this was originally to make all the transaction interface the same.
Also, composeTransactions relies on this.
We should refactor it but prefer to do it in the separate issue

Copy link
Contributor

Choose a reason for hiding this comment

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

and the composeTransactions was designed to address this exceptions if i understand correctly? I don't have the context though. but if it doesn't make sense now, its better to clean it up now. otherwise you can ignore this comment 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

composeTransactions was just a way to handle multiple cases for transaction validation and keeping the context while injecting to the transaction pool.
Ill create an issue to address this

Copy link
Member Author

Choose a reason for hiding this comment

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

Created: #5088

@shuse2 shuse2 merged commit 7ad636c into development Apr 2, 2020
@shuse2 shuse2 deleted the 5074-remove_old_vote branch April 2, 2020 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove old vote transaction
3 participants