AschPlatform / asch-js

Asch frontend library
18 stars 15 forks source link

add code for proposals and gateways #21

Closed bassjobsen closed 6 years ago

bassjobsen commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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.2upvote->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 commented 6 years ago

@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 https://github.com/AschPlatform/asch/issues/224#issuecomment-416205654 .

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 commented 6 years ago

@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 🙂