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

Cleanup JS code, added Docker configuration, added read example #9

Merged
merged 17 commits into from
Dec 22, 2021

Conversation

tgrospic
Copy link
Collaborator

Overview

This PR clean-up the JS code with RNode client helper which is used in all scripts to setup Rholang contracts for on-chain DB and runner which exports messages from Zulip.

Docker-compose configuration for RNode and Zulip and .env file are added to the project to make it easier to run.

Script tasks are moved from Make file to npm scripts for easier control and less OS dependency.

Added example script to read tables, keys and messages from on-chain DB.

Root README file is updated with instructions how to run the whole process. README file inside src related to detailed information about Zulip connection and DB triggers is not updated.

Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Thanks for polishing up my hack-n-slash code!

I have a few suggestions / comments / questions but nothing critical. This is an improvement as is.

"node": ">=14.4.0"
},
"dependencies": {
"@grpc/grpc-js": "^1.4.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason to use GRPC? If not, let's add an issue to get rid of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's required for gRPC communication. There are a few compatible variants available like grpc and grpc-web.

Copy link
Contributor

Choose a reason for hiding this comment

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

but do we need to do any gRPC communication? Are we doing anything that's not available via the web api?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are only using gRPC without any HTTP connection to the node.

Comment on lines 23 to 21
password: 'REPLACE_WITH_SECURE_POSTGRES_PASSWORD',
password: process.env.POSTGRES_PASSWORD,
Copy link
Contributor

Choose a reason for hiding this comment

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

ambient authority grumble...


// Load .env file
import { config } from 'dotenv';
config();
Copy link
Contributor

Choose a reason for hiding this comment

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

ambient authority grumble...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have any nice solution for this. dotenv uses fs and path directly and I didn't find some more SES compatible variant.

Do you have any suggestions? Is it possible to use SES shim to load it inside separate Compartment with provided fs and path as only global variables?

Copy link
Contributor

Choose a reason for hiding this comment

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

... dotenv uses fs and path directly ...

yeah; that's a bug. reimplementing dotenv with ocap discipline is probably about 20 lines of code. But it can wait for a later PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use SES shim to load it inside separate Compartment with provided fs and path as only global variables?

Yes, that can work too, if you really have to use 3rd party code and you can't change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah; that's a bug. reimplementing dotenv with ocap discipline is probably about 20 lines of code. But it can wait for a later PR.

👍 I wonder should it be update of existing lib or a new package?


I tried to import dotenv in isolated Compartment.

import 'ses';
lockdown();

const c = new Compartment({fs, path});

const r = c.evaluate(`
  import { config } from 'dotenv';
  config();
`);

But I see this error:

(SyntaxError#1)
SyntaxError#1: Cannot use import statement outside a module

  at Object.eval (eval at makeEvaluateFactory (file:///home/tomi/projects/rchain/rchat/node_modules/ses/dist/ses.umd.js:1400:10), <anonymous>:8:30)

Copy link
Contributor

Choose a reason for hiding this comment

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

should it be update of existing lib or a new package?

I would probably just add a .js file in this project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot use import statement outside a module

right... your browser will tell you the same thing if you try to eval an import declaration.

To load modules into a Compartment, you have to use @endo/compartment-mapper. Either importLocation or writeArchive followed by importArchive, I think. I asked for clarification in endojs/endo#963

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue to track updates #12.


// Load .env file
import { config } from 'dotenv';
config();
Copy link
Contributor

Choose a reason for hiding this comment

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

ambient authority grumble...

console.log(`Copy URI to .env file IDDB_CONTRACT_URI variable.`);
};

await main(process.env, {fs, grpcLib});
Copy link
Contributor

Choose a reason for hiding this comment

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

yay for passing process.env explicitly.


// Load .env file
import { config } from 'dotenv';
config();
Copy link
Contributor

Choose a reason for hiding this comment

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

ambient authority grumble...

src/iddb.rho Outdated
loop!(rest, acc.put(target, record))
loop!(rest, acc.set(target, record))
Copy link
Contributor

Choose a reason for hiding this comment

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

ugh. I clearly never tested this.

Bonus points for updating the "unit test" stuff below to exercise this code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kudos for having only one error without testing it. :)

Copy link
Collaborator Author

@tgrospic tgrospic Dec 20, 2021

Choose a reason for hiding this comment

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

@dckc Added some basic tests 32cfd17.

@@ -94,3 +99,18 @@ function makeGetDeployResult({ deployService }) {
return listenData;
};
}

/**
* @param {Object} arg
Copy link
Contributor

Choose a reason for hiding this comment

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

My habit is to call this io.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What you mean by io in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean:

Suggested change
* @param {Object} arg
* @param {Object} io

deployService is a form of IO. Most powerful stuff that we pass around is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I can see your perspective as argument that can serve IO operations. I'm looking more as function input arguments.


// Load .env file
import { config } from 'dotenv';
config();
Copy link
Contributor

Choose a reason for hiding this comment

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

ambient authority grumble...

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