Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish ENSIP-19 integration #95

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Finish ENSIP-19 integration #95

wants to merge 13 commits into from

Conversation

stevieraykatz
Copy link
Collaborator

No description provided.

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Oct 2, 2024

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@cb-heimdall
Copy link
Collaborator

Review Error for HandyPicklesStudioz @ 2024-10-02 04:17:46 UTC
User must have write permissions to review

Copy link
Collaborator

@cb-elileers cb-elileers left a comment

Choose a reason for hiding this comment

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

Overall this looks really good! Left a couple notes on Natspec updates that should be fixed, but once that's done I can give an approval.

uint256 discount;
}

struct URCStorage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing Natspec

/// @param reverseRegistrar_ The reverse registrar contract.
/// @param owner_ The permissioned address initialized as the `owner` in the `Ownable` context.
/// @param rootNode_ The node for which this registrar manages registrations.
/// @param rootName_ The name of the root node which this registrar manages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Natspec missing for params:
address paymentReceiver_,
address legacyRegistrarController_,
address reverseResolver_

view
returns (uint256 price)
{
URCStorage storage $ = _getURCStorage();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be done in a single line since $ is only called once in this function

Copy link
Collaborator

Choose a reason for hiding this comment

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

Leaving these up to discussion, non-blocking. This is more of a code-style question for consistency.

///
/// @param name The specified name.
/// @param resolver The resolver to set the reverse record on.
/// @param owner The owner of the reverse record.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing natspec for params:
uint256 expiry,
bytes memory signature

bytes memory signature
) internal {
URCStorage storage $ = _getURCStorage();
// vesitigial reverse resolution
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: vesitigial -> vestigial

@cb-heimdall
Copy link
Collaborator

Review Error for cb-elileers @ 2024-10-11 00:10:04 UTC
User failed mfa authentication, public email is not set on your github profile. see go/mfa-help

@cb-heimdall
Copy link
Collaborator

Review Error for OKEAMAH @ 2024-10-22 23:41:40 UTC
User must have write permissions to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants