Support dynamic plugin loading; Fp implementation with Ts compatibility; Mechanisms to revoke authorizations(#6) #7
Conversation
chfanghr
changed the base branch from
develop
to
connor/simple-password-tree-root-update
chfanghr
force-pushed
the
connor/dyn-plugins
branch
2 times, most recently
from
80ed5d0 to
173200b
Compare
adamczykm
left a comment
adamczykm
left a comment
There was a problem hiding this comment.
Please address the following:
https://www.notion.so/Avoid-the-use-of-fp-ts-in-publicly-facing-code-1b7c24afdab846599b52e99ebd09e6e7?pvs=4
|
Additionally, please, as always, add comments or even more beautiful documentation, we will need to do it anyway. Soon. Also they provide a lot of value and are cheap to produce. |
chfanghr
force-pushed
the
connor/dyn-plugins
branch
2 times, most recently
from
796859a to
7e94291
Compare
fix npm run scripts fix plugin loading fix custom route installation fix inconsistent log messages compatible with standard ts
chfanghr
force-pushed
the
connor/dyn-plugins
branch
from
7e94291 to
feb75b2
Compare
chfanghr
changed the title
chfanghr
force-pushed
the
connor/dyn-plugins
branch
from
b4bbec1 to
845192a
Compare
| (r) => T.fromIO(() => res.status(200).json(r)) | ||
| ) | ||
| ), | ||
| T.map(() => {}) |
There was a problem hiding this comment.
Basically, if you write out the type of the resulting lambda it would be (_: TaskEither<string, Express>): TaskEither<string, void>.
| // `${this.cfg.baseUrl}/getWitness/${treeRoot | ||
| // .toBigInt() | ||
| // .toString()}/${leafIndex.toString()}`; | ||
| // fetchPublicInputs( |
There was a problem hiding this comment.
Why this is left commented out?
There was a problem hiding this comment.
It's from the previously pr: #2 (comment)
adamczykm
left a comment
adamczykm
left a comment
There was a problem hiding this comment.
I will use a second-hand opinion on whether to ban the fp-ts usage.
It is not worth it in my opinion.
The library slowly starts to be mainly about types and how to deal with them which obscures a functionality that in fact is quite simple. The introduction of the interface kind made it even worse.
Additionally the code lacks commentary, especially given how hard it is to read now.
If you compose pipe operations in a foldleft and recurse, that is something that begs for a little bit of explanation of what is going on.
I'm sorry I don't know what to do with this PR.
| async (tree: TreeStorage) => { | ||
| const leaves = A.map(O.toUndefined)(await tree.getLeaves()); | ||
|
|
||
| // '/getWitness/:treeRoot/:leafIndex': (req, resp): Promise<void> => { |
| this.storageProvider.getTree(treeRoot), | ||
| TE.chain( | ||
| TE.fromOption( | ||
| () => `tree with root ${treeRoot.toString()} missing` |
There was a problem hiding this comment.
The amount of code bloat that the library generates is enormous.
Forbid the use of exceptions with the help of
fp-ts'sTaskEither. Base on #2.