Skip to content

Conversation

@Le-Caignec
Copy link
Contributor

No description provided.

@Le-Caignec Le-Caignec self-assigned this May 23, 2025
@Le-Caignec Le-Caignec changed the title Feature/createX Feat: createX May 26, 2025
Le-Caignec and others added 19 commits May 26, 2025 14:26
@Le-Caignec Le-Caignec marked this pull request as ready for review June 4, 2025 15:15
Copy link
Contributor Author

@Le-Caignec Le-Caignec left a comment

Choose a reason for hiding this comment

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

I created a new library named RLCOFTDeployer to factorize the code shared between the deployment functions of the RLCOFT and RLCOFTMock contracts, as they perform very similar operations.

Comment on lines +36 to +40
- name: Run Forge unit tests
run: make unit-test

- name: Run Forge e2e tests
run: make e2e-test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split tests for better issues isolation

address createXFactory,
bytes32 salt
) public returns (address) {
return RLCOFTDeployer.deployRLCOFT(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make the oposit : use Deploy.deploy insted of creating a new library only for this function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library is used in two places: once in the script/RLCOFT and once here.
Its purpose is to mutualize the deployment logic of both OftMock and OFT contracts, which share the same constructor and initialize parameters.
By centralizing this logic, the library helps factorize the deployment code and avoid duplication.

Note: An abstract contract could serve the same purpose. However, I chose to use a library for this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this answer your question ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I propose a new architcture in comment in the library, where we could use the lib in 3 places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i fixe it

@Le-Caignec Le-Caignec requested a review from gfournierPro June 6, 2025 08:30
Copy link
Contributor

@gfournierPro gfournierPro left a comment

Choose a reason for hiding this comment

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

great thanks a lot

@Le-Caignec Le-Caignec merged commit 901ad5a into main Jun 6, 2025
1 check passed
@Le-Caignec Le-Caignec deleted the feature/use-createX-for-deployment branch June 6, 2025 09:24
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.

3 participants