[Ready for review] New web3 support #4

nxtlvlrob · 2018-07-23T04:52:41Z

Key changes

Added a function initializeWindowWeb3 into /light/ which checks for existence of window.web3 and returns the appropriate status. (not sure how to test this one)
Added a function submitEd25519Delegation into /light/ which is uses axios to post the delegation request to the API.
Also added a helper function to utils genRandomHex - was running into the issue of random hex lengths not being long enough. See screenshot + this is currently an open issue on web3.js - web3.utils.randomHex does not produce consistent length strings web3/web3.js#1490

…void bug

XertroV · 2018-07-25T01:12:57Z

Added a function initializeWindowWeb3 into /light/ which checks for existence of window.web3 and returns the appropriate status. (not sure how to test this one)

Stuff like this (impure, etc) is okay not to test programatically (i.e. test by inspection) for the moment

XertroV · 2018-07-25T01:13:47Z

@rob I've also added https://github.com/secure-vote/sv-lib/blob/master/CONTRIBUTING.md recently - okay for the moment but in future do not commit changes from /dist or /docs (particularly because it's /lib not /dist now). I'll remove them so all okay in that regard.

edit: heh, seems like you have /lib and /dist in there :P

XertroV · 2018-07-25T01:14:12Z

dist/smart_contracts/UnsafeEd25519Delegation.abi.json

@@ -0,0 +1,280 @@
+[


This file should be minified

XertroV · 2018-07-25T01:15:10Z

src/.external-ecmascript.js

@@ -0,0 +1,192 @@
+// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects


this file shouldn't be committed AFAIK

XertroV · 2018-07-25T01:15:47Z

src/ballotBox.ts

@@ -239,7 +239,11 @@ export const prepareWeb3BBVoteTx = async ({ txInfo }, { svNetwork }) => {
    const { web3 } = svNetwork

    assert.equal(web3Utils.isAddress(bbFarm), true, 'BBFarm address supplied is not a valid ethereum address.')
-    assert.equal(web3Utils.isAddress(userAddress), true, 'User address supplied is not a valid ethereum address.')
+    // assert.equal(
+    //     web3Utils.isAddress(userAddress),


Why is this commented out?

XertroV · 2018-07-25T01:16:56Z

src/light.ts

+        return { detected: false, loaded: true }
+    }
+    const network = await detectNetwork(windowWeb3.currentProvider)
+    const _supported = network.id == 1 || network.id == 42


Not so sure about this... feels icky and what about classic, ropsten, etc? Shoudl probs be based off our list of known networks rather than hardcoded

XertroV · 2018-07-25T01:17:50Z

src/light.ts

@@ -243,7 +280,7 @@ export const stellarPkToHex = (pubKey: string): string => {
    } else {
        const kp = StellarBase.Keypair.fromPublicKey(pubKey)
        const rawPubKey = kp.rawPublicKey()
-        const hexPubKey = '0x' + rawPubKey.toString('hex')
+        hexPubKey = '0x' + rawPubKey.toString('hex')


variable not declared - any reason for removing 'const'?

XertroV · 2018-07-25T01:18:24Z

src/light.ts

@@ -261,15 +298,15 @@ export const getUnsafeEd25519Delegations = async (pubKey: string, svNetwork) =>
    const { unsafeEd25519DelegationAddr } = svConfig

    const Ed25519Del = new web3.eth.Contract(UnsafeEd25519DelegationAbi, unsafeEd25519DelegationAddr)
+    const hexPubKey = stellarPkToHex(pubKey)
    const delegations = await Ed25519Del.methods
        .getAllForPubKey(stellarPkToHex(pubKey))


if you declare hexPubKey here you should probably use it..

XertroV · 2018-07-25T01:18:45Z

src/light.ts

@@ -360,3 +386,34 @@ export const ed25519DelegationIsValid = (dlgtRequest: string, pubKey: string, si
    // Verify the request against the signature
    return kp.verify(dlgtRequest, sigBuffer)
 }
+
+export const submitEd25519Delegation = async (SvNetwork: any, dlgtRequest: string, pubKey: string, signature: string) => {
+    return new Promise((resolve, reject) => {


antipattern - promise in async function

XertroV · 2018-07-25T01:19:29Z

src/light.ts

@@ -360,3 +386,34 @@ export const ed25519DelegationIsValid = (dlgtRequest: string, pubKey: string, si
    // Verify the request against the signature
    return kp.verify(dlgtRequest, sigBuffer)
 }
+
+export const submitEd25519Delegation = async (SvNetwork: any, dlgtRequest: string, pubKey: string, signature: string) => {


also any time you have const f = () => { return dosomething() } you should write const f = () => dosomething() instead unless there's good reason.

XertroV · 2018-07-25T01:20:44Z

src/utils.ts

+
+export const genRandomHex = (bytes: number) => {
+    let randomHex = null
+    do {


do {} while () is super ugly and rarely used because it's harder to read; we should refactor this to be a bit more intelligent; e.g genRandomHex = (nBytes) => web3Utils.randomHex(nBytes + 10).slice(0, nBytes * 2 + 2)

XertroV · 2018-07-25T01:21:13Z

test/light.test.ts

@@ -5,3 +5,25 @@ import * as C from '../src/const'
 test('prepareEd25519Delegation works', () => {
    expect(L.prepareEd25519Delegation(C.zeroAddr, 'ffffff')).toBe('0x53562d45442d455448ffffff0000000000000000000000000000000000000000')
 })
+
+test('submitEd25519Delegation works', async () => {
+    const SvNetwork = {


Should have constants outside testing functions to avoid redelcaring them

XertroV · 2018-07-25T01:21:49Z

test/light.test.ts

+    const tSigLong =
+        '008c060ba60acad6594058e19e8d386adc4d4e142c3c44c140b4f15a331218abf9a5a11ef5022386a2174968f88f93fc246814b1097fb793d06b376d3452157104'
+
+    await expect(L.submitEd25519Delegation({}, tReqLong, tPk, tSig)).rejects.toThrow()


are there any tests for a successful request?

XertroV · 2018-07-25T03:16:10Z

src/const.ts

 }

+export const doesNetHaveIndex = (networkId, chainId) => getNetwork(networkId, chainId).indexEnsName !== undefined
+


Oops, we should alsocheck if its an empty string

Robert Scott added 3 commits July 23, 2018 14:37

added function to initialize web3 in the browser

6f74ae5

resolving conflicts

1529623

changes updated to lib

9a18337

nxtlvlrob requested a review from XertroV July 23, 2018 04:52

nxtlvlrob changed the title ~~New web3 support~~ [WIP] New web3 support Jul 23, 2018

Robert Scott added 3 commits July 23, 2018 14:56

added web3 utils

be8e585

resolved error where existing defined variable was declared again

d25d916

added function to submit delegation to API, hex generator helper to a…

216956c

…void bug

nxtlvlrob changed the title ~~[WIP] New web3 support~~ [Ready for review] New web3 support Jul 23, 2018

removed some console.log statements and tidied up a tiny bit

c71b5ce

XertroV self-assigned this Jul 25, 2018

XertroV reviewed Jul 25, 2018

View reviewed changes

dist/smart_contracts/UnsafeEd25519Delegation.abi.json Outdated

@@ -0,0 +1,280 @@

[

Copy link

Member

XertroV Jul 25, 2018

Choose a reason for hiding this comment

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

This file should be minified

Remove /dist and /lib

8487f1a

XertroV requested changes Jul 25, 2018

View reviewed changes

XertroV and others added 10 commits July 25, 2018 11:26

remove external-ecmascript file

c325517

updates as per PR feedback

e718c50

updates from max

92a0e1b

pulled updates and committing

b9af21d

Remove lib and docs

84f9402

add CNAME back for docs

c0bb5fb

add docs and lib as in master (git checkout master -- docs lib)

2925746

Add tests, deps, and fix up broken doesNetHaveIndex

acc791a

Move ApiError

057a9de

Add DecodeError and errtype param to checkDecode

230fc8a

XertroV approved these changes Jul 25, 2018

View reviewed changes

XertroV merged commit e089e09 into master Jul 25, 2018

XertroV deleted the new-web3-support branch July 25, 2018 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ready for review] New web3 support #4

[Ready for review] New web3 support #4

nxtlvlrob commented Jul 23, 2018 •

edited

XertroV commented Jul 25, 2018

XertroV commented Jul 25, 2018 •

edited

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018 •

edited

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

XertroV Jul 25, 2018

		@@ -0,0 +1,192 @@
		// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects

		}

		export const doesNetHaveIndex = (networkId, chainId) => getNetwork(networkId, chainId).indexEnsName !== undefined

[Ready for review] New web3 support #4

[Ready for review] New web3 support #4

Conversation

nxtlvlrob commented Jul 23, 2018 • edited

XertroV commented Jul 25, 2018

XertroV commented Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XertroV Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nxtlvlrob commented Jul 23, 2018 •

edited

XertroV commented Jul 25, 2018 •

edited

XertroV Jul 25, 2018 •

edited