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

Review chain helpers - Closes #3035 #3055

Merged
merged 21 commits into from
Mar 14, 2019
Merged

Conversation

diego-G
Copy link

@diego-G diego-G commented Mar 12, 2019

Review checklist

shuse2
shuse2 previously approved these changes Mar 12, 2019
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.

LGTM just small question

@diego-G diego-G requested a review from MaciejBaj March 12, 2019 13:52
Copy link
Contributor

@MaciejBaj MaciejBaj left a comment

Choose a reason for hiding this comment

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

Nice, good work so far.
What's missing:

helpers/index.js is outdated and can be removed
helpers/diff.js - maybe remove or the name should at least describe the operation on votes
helpers/transaction_types - I think should be moved to constants
helpers/z_schema is now duplicated with framework/src/controller/helpers/validator, we should use the formats defined there and only import them into z_schema

@2snEM6
Copy link
Contributor

2snEM6 commented Mar 12, 2019

@diego-G, regarding @MaciejBaj comment, let me know if you do something with helpers/transaction_types.js, maybe it solves code duplication on HTTP API too.

@shuse2
Copy link
Collaborator

shuse2 commented Mar 13, 2019

Also, I realised that slot.js is left behind in the helper, can we move it to logic or to appropriate round file?

@diego-G
Copy link
Author

diego-G commented Mar 13, 2019

@shuse2 helper/slots.js is used in many places. There are several logics, several submodules and two api controllers that make use of that util. I reckon it should be kept as a helper.

@diego-G diego-G requested review from shuse2 and 2snEM6 March 13, 2019 10:22
@shuse2
Copy link
Collaborator

shuse2 commented Mar 14, 2019

As discussed, please move the slot file to the logic folder for now

@diego-G
Copy link
Author

diego-G commented Mar 14, 2019

@shuse2 due to several hurdles and the current PR's size, I decide to open an independent issue to tackle your suggestion about helper slots. #3070

@diego-G diego-G requested a review from 2snEM6 March 14, 2019 10:47
@MaciejBaj MaciejBaj merged commit 1bd70fa into development Mar 14, 2019
@MaciejBaj MaciejBaj deleted the 3035-review_chain_helpers branch March 14, 2019 13:41
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.

Review chain/helpers/ content
4 participants