Skip to content

Commit

Permalink
Remove unused return value from _check() method (#24)
Browse files Browse the repository at this point in the history
<!-- Please refer to our CONTRIBUTING documentation for any questions on
submitting a pull request. -->
<!-- Provide a general summary of your changes in the Title above. -->

## Description

This PR removes the unused return value from `_check()` method from
`Excubia` abstract contract. See this [comment for
more](#20 (comment)).

<!-- Describe your changes in detail. -->
<!-- You may want to answer some of the following questions: -->
<!-- What kind of change does this PR introduce?** (Bug fix, feature,
docs update, ...) -->
<!-- What is the current behavior?** (You can also link to an open issue
here) -->
<!-- What is the new behavior (if this is a feature change)? -->
<!-- Does this PR introduce a breaking change?** (What changes might
users need to make in their application due to this PR?) -->

## Related Issue(s)
none
<!-- This project accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- Please link to the issue(s) here -->

<!-- Closes # -->
<!-- Fixes # -->

## Other information
none
<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
<!-- Feel free to remove this section if you will not use it. -->

## Checklist

<!-- Please check if the PR fulfills these requirements. -->

-   [x] My code follows the style guidelines of this project
-   [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
-   [x] My changes generate no new warnings
- [x] I have run `yarn format` and `yarn compile` without getting any
errors
- [x] I have added tests that prove my fix is effective or that my
feature works
-   [x] New and existing unit tests pass locally with my changes
  • Loading branch information
0xjei authored Jul 3, 2024
1 parent 2156bde commit 2641e84
Show file tree
Hide file tree
Showing 8 changed files with 11 additions and 27 deletions.
8 changes: 4 additions & 4 deletions packages/excubiae/contracts/Excubia.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,16 @@ abstract contract Excubia is IExcubia, Ownable(msg.sender) {
}

/// @inheritdoc IExcubia
function check(address passerby, bytes calldata data) external view returns (bool) {
return _check(passerby, data);
function check(address passerby, bytes calldata data) external view {
_check(passerby, data);
}

/// @notice Internal function to enforce the custom gate passing logic.
/// @dev Calls the `_check` internal logic and emits the relative event if successful.
/// @param passerby The address of the entity attempting to pass the gate.
/// @param data Additional data required for the check (e.g., encoded token identifier).
function _pass(address passerby, bytes calldata data) internal virtual {
if (!_check(passerby, data)) revert AccessDenied();
_check(passerby, data);

emit GatePassed(passerby, gate);
}
Expand All @@ -55,5 +55,5 @@ abstract contract Excubia is IExcubia, Ownable(msg.sender) {
/// @dev Custom logic to determine if the passerby can pass the gate.
/// @param passerby The address of the entity attempting to pass the gate.
/// @param data Additional data that may be required for the check.
function _check(address passerby, bytes calldata data) internal view virtual returns (bool) {}
function _check(address passerby, bytes calldata data) internal view virtual {}
}
6 changes: 1 addition & 5 deletions packages/excubiae/contracts/IExcubia.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ interface IExcubia {
/// @notice Error thrown when the gate address has been already set.
error GateAlreadySet();

/// @notice Error thrown when access is denied by the excubia.
error AccessDenied();

/// @notice Error thrown when the passerby has already passed the gate.
error AlreadyPassed();

Expand All @@ -45,6 +42,5 @@ interface IExcubia {
/// @dev Defines the custom gate protection logic.
/// @param passerby The address of the entity attempting to pass the gate.
/// @param data Additional data that may be required for the check.
/// @return True if the passerby passes the check, false otherwise.
function check(address passerby, bytes calldata data) external view returns (bool);
function check(address passerby, bytes calldata data) external view;
}
5 changes: 1 addition & 4 deletions packages/excubiae/contracts/extensions/EASExcubia.sol
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ contract EASExcubia is Excubia {
/// @dev Checks if the attestation matches the schema, attester, recipient, and is not revoked.
/// @param passerby The address of the entity attempting to pass the gate.
/// @param data Additional data required for the check (e.g., encoded attestation ID).
/// @return True if the attestation is valid and the passerby passes the check, false otherwise.
function _check(address passerby, bytes calldata data) internal view override returns (bool) {
function _check(address passerby, bytes calldata data) internal view override {
super._check(passerby, data);

bytes32 attestationId = abi.decode(data, (bytes32));
Expand All @@ -77,7 +76,5 @@ contract EASExcubia is Excubia {
if (attestation.attester != ATTESTER) revert UnexpectedAttester();
if (attestation.recipient != passerby) revert UnexpectedRecipient();
if (attestation.revocationTime != 0) revert RevokedAttestation();

return true;
}
}
5 changes: 1 addition & 4 deletions packages/excubiae/contracts/extensions/ERC721Excubia.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,12 @@ contract ERC721Excubia is Excubia {
/// @dev Checks if the passerby is the owner of the token.
/// @param passerby The address of the entity attempting to pass the gate.
/// @param data Additional data required for the check (e.g., encoded token ID).
/// @return True if the passerby owns the token, false otherwise.
function _check(address passerby, bytes calldata data) internal view override returns (bool) {
function _check(address passerby, bytes calldata data) internal view override {
super._check(passerby, data);

uint256 tokenId = abi.decode(data, (uint256));

// Check if the user owns the token.
if (!(NFT.ownerOf(tokenId) == passerby)) revert UnexpectedTokenOwner();

return true;
}
}
5 changes: 1 addition & 4 deletions packages/excubiae/contracts/extensions/FreeForAllExcubia.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,7 @@ contract FreeForAllExcubia is Excubia {
/// @dev This function always returns true, signaling that any passerby is able to pass the gate.
/// @param passerby The address of the entity attempting to pass the gate.
/// @param data Additional data required for the check (e.g., encoded attestation ID).
/// @return True, allowing any passerby to pass the gate.
function _check(address passerby, bytes calldata data) internal view override returns (bool) {
function _check(address passerby, bytes calldata data) internal view override {
super._check(passerby, data);

return true;
}
}
3 changes: 1 addition & 2 deletions packages/excubiae/test/EASExcubia.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,8 @@ describe("EASExcubia", function () {
})

it("should pass the check", async () => {
const passed = await easExcubia.check(signerAddress, validAttestationId)
await expect(easExcubia.check(signerAddress, validAttestationId)).to.not.be.reverted

expect(passed).to.be.true
// check does NOT change the state of the contract (see pass()).
expect(await easExcubia.registeredAttestations(validAttestationId)).to.be.false
})
Expand Down
3 changes: 1 addition & 2 deletions packages/excubiae/test/ERC721Excubia.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,8 @@ describe("ERC721Excubia", function () {
})

it("should check", async () => {
const passed = await erc721Excubia.check(signerAddress, encodedValidTokenId)
await expect(erc721Excubia.check(signerAddress, encodedValidTokenId)).to.not.be.reverted

expect(passed).to.be.true
// check does NOT change the state of the contract (see pass()).
expect(await erc721Excubia.registeredTokenIds(rawValidTokenId)).to.be.false
})
Expand Down
3 changes: 1 addition & 2 deletions packages/excubiae/test/FreeForAllExcubia.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,8 @@ describe("FreeForAllExcubia", function () {
describe("check()", function () {
it("should check", async () => {
// `data` parameter value can be whatever (e.g., ZeroHash default).
const passed = await freeForAllExcubia.check(signerAddress, ZeroHash)
await expect(freeForAllExcubia.check(signerAddress, ZeroHash)).to.not.be.reverted

expect(passed).to.be.true
// check does NOT change the state of the contract (see pass()).
expect(await freeForAllExcubia.registeredPassersby(signerAddress)).to.be.false
})
Expand Down

0 comments on commit 2641e84

Please sign in to comment.