Skip to content
This repository has been archived by the owner on May 19, 2023. It is now read-only.

feat: batch support for domain registration #293

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Conversation

julianmrodri
Copy link
Contributor

@julianmrodri julianmrodri commented Sep 9, 2020

  • Decode domain names when processed in Batch. This requires processing the internal transactions of the TX data field, using RLP and following the FifsRegistrar encoding.)
  • Add support and configuration for the RNS Batch Registration contract (in all environments).
  • Test for transaction decoding in Batch Mode (Registering two domains).

@julianmrodri julianmrodri reopened this Sep 10, 2020
@julianmrodri julianmrodri marked this pull request as ready for review September 10, 2020 02:29
Copy link
Member

@jurajpiar jurajpiar left a comment

Choose a reason for hiding this comment

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

🚀 Looking good

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@AuHau AuHau left a 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 vs batchProcess)
  • 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.

@AuHau AuHau changed the title Feat/batchsupport feat: batch support for domain registration Sep 15, 2020
@AuHau AuHau merged commit 971fb7e into release/mainnet Sep 15, 2020
@AuHau AuHau deleted the feat/batchsupport branch September 15, 2020 08:47
@AuHau AuHau mentioned this pull request Sep 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants