-
Notifications
You must be signed in to change notification settings - Fork 3
feat: batch support for domain registration #293
Conversation
4aa8142
to
ed21b98
Compare
4e83be4
to
45d33fe
Compare
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.
🚀 Looking good
45d33fe
to
2f7ff21
Compare
Kudos, SonarCloud Quality Gate passed!
|
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 have taken the liberty to do small refactor for some style tweaks as I did not find any critical problems.
I have few general remarks though:
- please don't force push once a PR is reviewed, it makes it hard to track the incremental changes for the reviewer
- use
camelCase
for variable naming (batchprocess
vsbatchProcess
) - if you want to write a comment for function use JSDoc (see example here
One more thing that we should generally improve. Whenever there is some "magic" thing that cannot be deduced from the codebase itself, there should be an exhaustive comment on why it is what it is. For example when you have name = Utils.hexToAscii('0x' + domainData.slice(218, domainData.length))
you should explain why do you slice
from 218
(for example what is the format of the hash/data etc.). Similarly, why do you decode the Batch registration in the way you do.
The problem here is that if somebody who does not have a clue what is this about comes to the code, he won't have any idea what is going on. In my experience, this can be even you once you come back to the code in a month or two, after you moved to do some other thing, so it is generally good to have it there. We can add it in separate PR, so it does not block the release.
FifsRegistrar
encoding.)