feat!: mantaray 1.0 #30
Conversation
There was a problem hiding this comment.
This is a big chunk of work, good job @nugaon ! 👏
I have focused my review mainly on the new version implementation (eq. the mantaray-v1 file) and ignored the tests for now. This will be an iterative process anyhow. Also here I focus on the implementation itself and not on the newly proposed protocol, where I also have some reservations but I will put them in comments directly to the SWIP.
Generally, it is going in the right direction. I have all sorts of comments from critical missing validations to fix, code readability improvements, clarification questions and naming suggestions. I also have a few general remarks.
One big thing I am missing is input validations. I know we have Typescript but that does not prevent passing in any input the user desire (or does it accidentally) which might lead to weird bugs. I have marked some of the places that I think the validation is missing with the comment "Type check". The list might not be complete so please go around all the public API and see where inputs could be validated.
Generally, I would welcome more comments explaining the context of the terms and actions so people without such good domain knowledge could navigate the code. Maybe links to some explaining resources could be also included (for example your SWIP).
To be honest I am not fond of how you handle this.forks. You have in so many places conditions like
if (this.isDirty() && !this.forks) this.forks = {}
I would suggest finding some better way how to go about it. There is one comment dedicated to that, so we can discuss it there.
Also to note. You are using recursion in processing a lot of the tasks. It has its limitation from the nature of the programming language. I was curious what are the limitations so I have searched a bit and found that JS has generally around 10k recursion depth (see for example here). This is a lower bound imposed mainly by browsers and for example, in Node this is possible to affect with runtime flags, but still it is a limitation.
I think that 10k recursion depth (eq. in Mantaray context it is the maximum depth of the Mantaray tree) is alright for general use cases but sooner or later somebody will hit that limit. This is a bit tricky because it is a language limitation so for example Bee (eq. Golang) might process some mantaray without issue, but JS implementation might have problems. I don't think it is something we have to deal right now, maybe we could add some error handling around that to better communicate what is happening when it is happening, but I am not so sure about that myself. Would love to hear your opinion on this.
| } | ||
|
|
||
| /** If the JSON deserialisation of the data is not succesful, it will give back undefined */ | ||
| export function deserializeMetadata(data: Uint8Array): MetadataMapping | undefined { |
There was a problem hiding this comment.
Why do you choose to ignore errors? I mean it either has to be empty or valid JSON no? Or is it because of the v0.2 compatibility?
There was a problem hiding this comment.
no compatibility issues here, just the bytesize always reserved for the JSON metadata that causes issue if there wasn't any metadata defined for the fork, because the zero bytearray not a valid stringified JSON object.
|
|
||
| /// BL methods | ||
|
|
||
| public addFork( |
There was a problem hiding this comment.
Could we simplify this method? It is super long. I would suggest to extract some of the case handling (like for example continuous node creation etc.) into separate (private) methods/utility functions?
There was a problem hiding this comment.
This part has still legacy from the Bee-side implementation keeping the same structure as it was implemented.
I also was thinking it may worth to refactor the logic but it could make harder the Bee-side integration if it is accepted.
| if (!equalBytes(this._obfuscationKey, null32Bytes)) { | ||
| if (!obfuscationKeyGenerator) { | ||
| throw new RandomBytesFnUndefined() | ||
| } | ||
| newNode.obfuscationKey = obfuscationKeyGenerator() | ||
| } |
There was a problem hiding this comment.
This is for example candidate for refactoring out to a separate method as it is even repeated several times.
There was a problem hiding this comment.
I think it is a bit overengineering the problem, there are only 2 occasions it appears and the share the same error class anyway.
This PR serves as a reference implementation of ethersphere/SWIPs#37 in file
src/mantaray-v1.ts.forkMetadataSegmentSizeis set automatically on node serialisation.forkReferencemove tocontentAddressfromentrywhen fork deserialisation happens.There are some other changes in the code-base that also affect the Mantaray v0.2 usage:
gen32Bytesutility function as it was not safe byte generation forobfuscationKey. If one wants to useobfuscationKeyencrypting, they have to generate the key outside of the library or pass 32 byte generate function to theaddForkMantarayNodeto distinguish the two not compatible Mantaray formats which replaced the old return value ofinitManifestNodeMantarayV1andMantarayV0_2objects.1.0Fixes #17 Resolves #16 Closes #32