Closed dckc closed 5 years ago
Thanks for the careful review. These are all good points.
On Sat, Nov 17, 2018, 9:53 PM Joshy Orndorff <notifications@github.com wrote:
@JoshOrndorff commented on this pull request.
In src/RHOCore.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234427744 :
@@ -85,7 +85,7 @@ function toJSData(par /: IPar /) /: Json /{ return ex.g_bool; } if (typeof ex.g_int !== 'undefined') {
- return ex.g_int;
- return parseInt(ex.g_int, 10); // ISSUE: overflow
What's the overflow issue? Integer overflow is documented as well as any other feature of rholang. I don't think JS should try to change that behavior.
In src/RHOCore.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234427768 :
@@ -94,6 +94,18 @@ function toJSData(par /: IPar /) /: Json /{ && Array.isArray(ex.e_list_body.ps)) { return ex.e_list_body.ps.map(recur); }
- if (typeof ex.e_map_body !== 'undefined' && ex.e_map_body !== null
- && Array.isArray(ex.e_map_body.kvs)) {
- const props = ex.e_map_body.kvs.map((kv) => {
- const key = recur(kv.key || {});
- if (typeof key !== 'string') {
- throw new Error(
not RHOCore? ${JSON.stringify(key)}
);- }
- const val = recur(kv.value || {});
- return { k: key, v: val };
- });
- return props.reduce((acc, { k, v }) => ({ [k]: v, ...acc }), {});
- }
Looks like you're adding rholang maps to RHOCore, but I'm not good enough at reading functional-style JS to understand what's happening here.
In src/proxy.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234427791 :
+/:: +import type { IRNode } from './rnodeAPI'; +/ + +exports.makePeer = makePeer; +function makePeer(rnode /: IRNode /, user /: Uint8Array /,
- clock /: () => Date /,
- setTimeout,
- // ISSUE: is 1/2 sec between createBlock and listen long enough?
- delay=500,
- payOpts={ phloLimit: 100000, phloPrice: 1, from: '0x01'}) {
- /**
- Note: For better compositionality, JS args are combined into one
- list arg on the rholang side.
- */
@param https://github.com/param target A string representing an RChain registry URI (backticks?) @param https://github.com/param method Name of the method you;d like to call from js @param https://github.com/param args Array or arguments to be passed into rholang contract @return https://github.com/return String of rholang code that executes the requested #transaction
In src/proxy.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234427846 :
- lookup!(`TARGET`, *targetCh) |
- for(target <- targetCh) {
- target!(${method}, ${args}, *return)
- }
- }
- `.replace('TARGET', target);
- console.log(term);
- return term;
- }
- async function returnChannel(timestamp) {
- const ids = await rnode.previewPrivateNames({ user, timestamp, nameQty: 1 });
- const id = ids.ids[0];
- return { ids: [{ id }] };
- }
@param https://github.com/param ....
In src/proxy.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234427878 :
@@ -0,0 +1,79 @@ +/global require, exports/ + +const { rhol, toJSData, fromJSData } = require('./RHOCore'); +const { h2b } = require('./signing'); +const { RNode } = require('./rnodeAPI'); + +const def = Object.freeze; + +/:: +import type { IRNode } from './rnodeAPI'; +/ + +exports.makePeer = makePeer; +function makePeer(rnode /: IRNode /, user /: Uint8Array /,
Did this lint? Do we need a new line after the first comma?
In test/testProxy.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234427921 :
+/**
- To run this test, first deploy target1.rho (and propose).
- It will register a contract and print out the registry URI.
- *
- Pass that URI on the command line.
- */
+async function test({ rnode, clock, user, setTimeout, target }) {
- const peer = makePeer(rnode, user, clock, setTimeout);
- const ansToSend = await peer.sendCall(target, "buy", ["orange", 20]);
- console.log({ ansToSend });
- const targetProxy = peer.makeProxy(target);
- const ansToProxy = await targetProxy.sell("banana", 20, 3);
That's amazing 💯
In src/proxy.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234428123 :
- args /: mixed[]/) {
- const term = callSource(target, method, args);
- const timestamp = clock().valueOf();
- const returnCh = await returnChannel(timestamp);
- await rnode.doDeploy({ term, user, timestamp, ...payOpts }, true);
- if (delay) {
- await makeTimer(setTimeout)(delay);
- }
- const blockResults = await rnode.listenForDataAtName(returnCh);
- // console.log({ blockResults });
- const answerPar = blockResults[0].postBlockData[0];
- // console.log('answerPar', JSON.stringify(answerPar, null, 2));
- return toJSData(answerPar);
- }
- function makeProxy(rName /: string /) {
/**
- Creates a javascript object that will dispatch method calls to the underlying rholang contract.
- @param https://github.com/param Not totally sure how to read this code
- @return https://github.com/return Proxy object */
Suggestion: optional param for list of enabled methods. If I know I'm making a proxy for a purse, then I'll pass in ['split', 'balance' ... ] so that if a user gets confused and calls 'set-status' by mistake, they will get a helpful error message.
In src/rnodeAPI.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234428128 :
nonce: number } type DeployDataInsecure = { term: string, timestamp: number, from: string,
- phloPrice: PhloPrice,
- phloLimit: PhloLimit,
- phloPrice: number,
- phloLimit: number, nonce: number
Should we call this version 0.7.2 since this change is not present in the release version of 0.7.1?
I vote yes.
In test/testProxy.js https://github.com/rchain-community/rchain-api/pull/49#discussion_r234428324 :
+ +const { makePeer, RNode, RHOCore, h2b } = require('..'); + +const { fromJSData } = RHOCore; +const def = Object.freeze; + +/**
- To run this test, first deploy target1.rho (and propose).
- It will register a contract and print out the registry URI.
- *
- Pass that URI on the command line.
- */
+async function test({ rnode, clock, user, setTimeout, target }) {
- const peer = makePeer(rnode, user, clock, setTimeout);
Making a peer only to later make separate proxies seems clunky. That may be because I don't really understand what user is and why you've chosen to make a peer encapsulate it. Why not just pass user in to the RNode or each proxy depending what it represents?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rchain-community/rchain-api/pull/49#pullrequestreview-176074480, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJNyqwnHxJ0FDJkYeRooLqyNnQ-tQyeks5uwNm3gaJpZM4Ynusa .
Yes, they're in the genesis block; 1b179ca in the payment branch (#51) addresses the symlink issue as such.
It would be cleaner to keep this proxy PR separate from the payment PR, but it would be more work. Maybe just close this PR?
Yeah, let's do it all as one PR.
The idea is demonstrated in
testProxy.js
:depends on #30