-
Notifications
You must be signed in to change notification settings - Fork 26
Implement single-module marine services deployment service #198
Conversation
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.
Proposed some small fixes, asked some questions
| export { callFunction as callFunction$$ } from './v3'; | ||
| export { registerService as registerService$$ } from './v3'; |
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.
Since you are exporting everything from './v3' you can move these two exports up as well
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.
fixed
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.
did you commit? I don't see the change
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.
yep :(
packages/fluence-js/src/__test__/integration/sigService.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| constructor(private peer: FluencePeer) {} | ||
|
|
||
| createSecurityGuard: SecurityGuard<'wasm_b64_content'> = defaultGuard(this.peer); |
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.
Shouldn't this be called defaultGuard or something?
Because seems like in the code you are using it, not creating it
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.
Create is the name of service's function create. I'll try to figure a better way of naming these guards
| import { CallParams } from '../commonTypes'; | ||
| import { allowOnlyParticleOriginatedAt, SecurityGuard } from './securityGuard'; | ||
|
|
||
| export const defaultGuard = (peer: FluencePeer) => { |
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 am just curious, could you please explain how this is working
Looks like this means that you will be able to create services only when runnning js code on nodejs by yourself. If you try to create service on some nodejs peer from outside, e.g. from browser client - it will fail. Right?
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.
Kinda. Basically the guard allows to you mess with services only when they are hosted on your peer and you initiate the particle on the same peer. So if you host something on your browser peer, only you will have the "write" access to it as well.
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 it temporary or a permanent solution? Did you discuss it with anyone? Maybe it would be cool if we are not as restrictive
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.
The original idea was proposed by @alari. TBH I'm not 100% sure what the best strategy regarding security. The general idea is that by default only the peer owner has control over service creation. But the owner can change the security guard according their use case
| success: false, | ||
| error: 'Security guard validation failed', | ||
| service_id: null, |
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 don't like these kind of result types, but I see we do these "success, error, result" in a several of our services
I don't know if there is a default approach to handle this Result kind of types in aqua
If it is considered to be a standard and good approach - don't know if it's a good idea but maybe there should be an exported js helper function Result which could look something like this
type AquaErr = {
success: false;
error: string;
} & {
[key in string]: null;
};
type AquaOk<R> = {
success: true;
error: null;
} & {
[key in string]: R;
};
class Result<R> {
constructor(public field: string) {
this.field = field;
}
err(err: string): AquaErr {
// @ts-ignore
return {
success: false,
error: err,
[this.field]: null,
};
}
ok(res: R): AquaOk<R> {
// @ts-ignore
return {
success: true,
error: null,
[this.field]: res,
};
}
}
new Result("id").ok(1);
new Result("id").err("Some error");I would rather prefer not to have success at all and for the code to look something like this:
type AquaErr = {
error: string;
};
type AquaOk<F extends string, R> = {
[key in F]: R;
};
class Result<F extends string, R> {
constructor(public field: F) {
this.field = field;
}
err(err: string): AquaErr {
return {
error: err,
};
}
ok(res: R): AquaOk<F, R> {
// @ts-ignore
return {
[this.field]: res,
};
}
}
new Result("id").ok(1);
new Result("id").err("Some error");Success to just mean there is no error
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.
@justprosh @folex please help me understand if we need explicit success in aqua and why
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.
That's the limitation of aqua language. No way to represent sum types. At least at the moment
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 understand there is no way to represent a sum type in aqua, but is success really required? Is it possible to understand success as absence of the error
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.
it's the limitation of marine and aqua, yes: we don't have good mechanics for errors yet.
success is more explicit and easier than checking error for emptiness.
besides, it could be that in some code error is not empty while success is true, or vice versa: result is not empty while success is false.
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 agree with @shamsartem here. For me having both error message and success value sounds like a poor API design. Nevertheless we desperately need to implement some sort of a sum type in aqua to solve this issue. At the moment I don't see any mechanism to implement such API design
|
|
||
| try { | ||
| // eval('require') is needed so that | ||
| // webpack will complain about missing dependencies for web target |
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.
Maybe add a TODO to possibly solve this with a dynamic import when we finally migrate to ES modules
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'm not sure webpack wouldn't complain in that case either :)
| if (!isNode) { | ||
| return { | ||
| success: false, | ||
| error: 'read_file is only supported in node.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.
There is filesystem access api for desktop browsers (bad support, we can ignore this for now) and also there is just a way to load binary files in browser using <input type="file"> so would be cool to support this in browser as well, @alari what do you think?
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.
This service is meant to access files from aqua without user interaction. I don't understand how we can leverage input forms
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.
The better approach would be using IPFS for file transfers. But that's not implemented in FJS directly yet
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 understand the purpose of this particular service. It's just that this error message got me thinking
I understand now that the thing that I wanted to do can be done by compiling Srv.create function and calling it in js directly
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.
Thanks
No description provided.