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

add code for proposals and gateways #21

Merged
merged 1 commit into from
Sep 17, 2018

Conversation

bassjobsen
Copy link
Contributor

No description provided.

bassjobsen added a commit to bassjobsen/asch-cli that referenced this pull request Aug 26, 2018
@bassjobsen bassjobsen changed the base branch from master to develop August 29, 2018 22:43
Copy link
Contributor

@a1300 a1300 left a comment

Choose a reason for hiding this comment

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

@bassjobsen please provide consistent identation. asch-js uses TABS as identation.

Thanks 👍

@bassjobsen
Copy link
Contributor Author

@a1300 please give an example of file and line number with wrong ident, because of for me it seems that the indetation is the same as the original files?

@a1300
Copy link
Contributor

a1300 commented Sep 2, 2018

@bassjobsen github doesn't render the whitespace in code, but I saw that it was off, because github is using another tabspace width.

If you install this browser extension: render-whitespace-on-github you can also view it in the browser.

Your https://github.com/AschPlatform/asch-js/blob/603b62ad0d8747ec5cc0a5b429755c0bd3b61cf7/lib/transactions/gateway.js file:
bassjobsen_gateway

Your https://github.com/AschPlatform/asch-js/blob/603b62ad0d8747ec5cc0a5b429755c0bd3b61cf7/lib/transactions/proposal.js file:
bassjobsen_proposal

Please only use TABS 👍

P.S: I configured Visual Studio Code to render whitespace:
vs_code_whitespace

@a1300
Copy link
Contributor

a1300 commented Sep 16, 2018

@bassjobsen I reviewed your pull request. I have only very small change proposals:

  1. Proposal.js Line 72
    1.1 Variable keys is not used. Variable can be removed
  2. For the propsal.upvote function, it would be easier to pass the transactionId as a variable, instead in the options object
    2.1 same for proposal.activate
  3. In proposal.js please rename
    3.1 activate -> activateProposal 3.2 upvote->upvoteProposal 3.3 Maybe you could also provide the functionrevokeProposal`
  4. For initgateway and I would make the name optional. Instead of hardcoding xxxxxxxxxx. You could make this parmaeter optional.

@bassjobsen
Copy link
Contributor Author

@a1300 About the renaming of the function activate -> activateProposal, that's more clear indeed. Th reaason i didn't name it activateProposal is that it is alread in a file called proposal.js.
About the proposal.revoke, yes you're right about that, i'm waiting for a response on AschPlatform/asch#224 (comment) .

And finally about the harded xxxxxxxxxx i will check that. In fact it mean the code of asch requires this parameter (not) optional and does not use it.

@a1300
Copy link
Contributor

a1300 commented Sep 17, 2018

@bassjobsen regarding the renaming: Normally I would also think that the file is called proposal.js and we don't need to name the function activate -> activateProposal but it is somehow misleading that in the proposal.js part are many gateway functions. So I thought we could make it perfectly clear.

I like that you structured the code like the asch-core repo 👍

Regarding the proposal.revoke: You could implement the revoke function and the gateway_update_member function. And if the proposal.unvote function gets implemented, you can provide another pull request!

https://github.com/AschPlatform/asch/blob/develop/src/contract/proposal.js

const VALID_TOPICS = [
  'gateway_register',
  'gateway_init',
  'gateway_update_member',
  'gateway_revoke',
]

I only told you about the hard coded xxxxxxxxxx because I saw it in the database. It is not very aesthetically pleasing that every init_gateway proposal has the same name 🙂

@liangpeili liangpeili merged commit 0ec5bd0 into AschPlatform:develop Sep 17, 2018
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.

3 participants