-
Notifications
You must be signed in to change notification settings - Fork 19
Web only version (gRPC related examples removed) #17
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
Conversation
| </head> | ||
| <body> | ||
| <div class="dev-info-box"> | ||
| This is testing version of the page. For official version please visit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
official? What's official about the other one? you might call it "production"
| * @param {string} hexStr | ||
| * @returns {string} | ||
| */ | ||
| export const encodeBase58 = hexStr => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A name like lib.js or utils.js is a sort of code smell.
I have been using hex.js that exports Basic16 with encode and decode static methods. (I think that particular copy only has encode, but you get the idea).
I suppose Base58.encode and .decode could belong in the same file if it were called codecs.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import { ec } from 'elliptic' | ||
| import { decodeBase16, encodeBase58, encodeBase16, decodeBase58safe } from './lib' | ||
|
|
||
| const secp256k1 = new ec('secp256k1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any mutable state inside this secp256k1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it seems to include a random number generator.
| const prefix = { coinId : "000000", version: "00" } | ||
|
|
||
| /** | ||
| * @typedef {Object} RevAccount - Represents different formats of REV address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also write this whole typedef in one line:
* @typedef { {privKey?: string, pubKey?: string, ethAddr?: string, revAddr: string } } RevAccount| */ | ||
| export const getAddrFromEth = ethAddrRaw => { | ||
| const ethAddr = ethAddrRaw.replace(/^0x/, '') | ||
| if (!ethAddr || ethAddr.length !== 40) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this typecheck with @returns { string }?
| */ | ||
| export const verifyRevAddr = revAddr => { | ||
| const revBytes = decodeBase58safe(revAddr) | ||
| if (!revBytes) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, does this typecheck? it should throw, no? Or change the return type to string | undefined.
| */ | ||
| export const newRevAccount = () => { | ||
| // Generate new key and REV address from it | ||
| const key = secp256k1.genKeyPair() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OUCH! ambient access to a random number generator!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised a separate issue: #18
| <input type=text autocomplete=nono placeholder=${labelSource} value=${text} oninput=${addrKeyPressEv} /> | ||
| <!-- New accounts --> | ||
| ${ethDetected && html` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OCAP-OUCH! global mutable state
already noted as #15
|
The htm stuff in 8d0aa4f is great! |
| /** | ||
| * These deploy types are based on protobuf specification which must be | ||
| * used to create the hash and signature of deploy data. | ||
| * Deploy object sent to Web API is slightly different, see [rnode-web.js](rnode-web.js). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice. I spent a lot of time puzzling over these types, looking at scala code, etc.
See also https://github.com/rchain-community/liquid-democracy/blob/dev-tools/rclient/types.js WIP
| // Add status if server error | ||
| if (!resp.ok) { | ||
| const ex = Error(result) | ||
| // @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I did the same thing. hm.
|
|
||
| // Make RNode web API client / wrapped around DOM fetch | ||
| const rnodeWeb = makeRNodeWeb({fetch}) | ||
| const rnodeWeb = makeRNodeWeb({fetch, now: Date.now}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent.
A few OCap no-nos remain, but those are orthogonal to these changes, so I'm raising separate issues.
|
I hope this lands soon. What's left to do? |
|
Subsumed by newer PR #21 with full TypeScript support. |
Also added htm library to write render functions with template literals which looks like HTML.