Skip to content

Add tests for Openzeppelin dependencies of feat/v3-implementation#4

Merged
rayedsikder merged 58 commits intotest/v3-implementationfrom
test/v3-implementation-deps
Oct 3, 2023
Merged

Add tests for Openzeppelin dependencies of feat/v3-implementation#4
rayedsikder merged 58 commits intotest/v3-implementationfrom
test/v3-implementation-deps

Conversation

@Suvadra-Barua
Copy link
Collaborator

This pull request introduces Foundry tests for CCP v3-implementation. There are eight OpenZeppelin dependencies utilized in CCP v3-implementation . These tests primarily focus on the main functions used in feat/v3-implementation.

  1. Context.t.sol
  • Test happy path of Context.sol
  • Add MockContext contract
  • Add test functions for the current execution context, including the sender of the transaction and its data
  1. Ownable.t.sol
  • Add tests for Ownable.sol which provides a basic access control mechanism
  • Add MockOwnable contract
  1. Pausable.t.sol
  • Add test functions for Pausable.sol which allows children to implement an emergency stop
    mechanism that can be triggered by an authorized account.
  • Include MockPausable for testing purposes
  1. Counters.t.sol
  • Add test functions for counters that can only be incremented, decremented or reset
  1. ERC20.t.sol
  • Add tests for ERC20.sol
  • Include tests for initial set up, transfer, transfer from, mint, approve, and burn functions.
  • Add ERC20Mock contract
  1. draft-ERC20Permit.t.sol
  • Add tests for draft-ERC20Permit.sol
  • Add MockERC20Permit contract
  • Add test to check permit function only
  1. ERC721.t.sol
  • Add tests for ERC721.sol
  • Add ERC721Mock to include _safeMint
  • Include tests for initial set up, supports interface, balance, owner, safe mint, transferFrom, approve functions.
  1. ERC721Burnable.t.sol
  • Add tests for ERC721Burnable.sol
  • Add ERC721BurnableMock contract
  • Add test to check burn function only

Copy link
Member

Choose a reason for hiding this comment

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

Context tests are interesting.
The tests are clean and well written. However, the approach I'd have taken is to directly inherit the Context in the test file. Otherwise, it is not composable. We need think of efficient ways of testing internal functions in such a way that when test files are utilized in other tests, the dependency test file do not need extra modification

Copy link
Member

@rayedsikder rayedsikder left a comment

Choose a reason for hiding this comment

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

All the tests passed.

LGTM except the Ownable.t.sol contract which needs minor changes to make it composable and compatible with main contracts (Otherwise the tests will fail when it will be run in context of main ccp-contracts)

On a different note, we need think of efficient ways to test internal functions without compromising composability.

Copy link
Member

Choose a reason for hiding this comment

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

Ownable.t.sol tests the abstract contract Ownable very well. However, it is not composable at this format because of use custom function names.

When Ownable is inherited the public and external functions are accessible to the inherited contract with the same names. Use of custom names transferOwnershipExternal & checkOwner reduces the composability.

The purpose of this OwnableTest contract would be to support main contracts, e.g - CampaignInfo. In that case, when with modified constructor parameters CampaignInfo will be created inside OwnableTest and transferOwnershipExternal & checkOwner will be called it will throw an error.

I would recommend to use the default public functions transferOwnership and owner. Similar to how you used renounceOwnership - which will work with any contract that inherits Ownable.

@rayedsikder rayedsikder merged commit 30ca6c8 into test/v3-implementation Oct 3, 2023
@rayedsikder rayedsikder deleted the test/v3-implementation-deps branch March 20, 2024 20:54
rayedsikder pushed a commit that referenced this pull request Dec 9, 2025
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.

2 participants