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

test(refactor): move all variables into Constants contract #71

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Feb 15, 2025

Closes #63

@smol-ninja smol-ninja force-pushed the tests/default-contract branch 2 times, most recently from 711e2e6 to c985d32 Compare February 15, 2025 20:44
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

my suggestion in that issue was to completely remove Defaults or import it into Base_Test, so we don’t have to call anything like defaults.<foo>

i don’t see the point of leaving some things in Defaults while moving others into Base_Test.

so i would rather:

  1. move the index<num>Proof functions back to Defaults and import it into Base_Test, or
  2. completely move the function from Defaults into Base_Test.

both work for me

@smol-ninja
Copy link
Member Author

Sounds good. I will move everything into Base. That looks a better design to me.

@smol-ninja
Copy link
Member Author

Included your suggestion of removing Defaults contract @andreivladbrg.

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

after seeing the actual functions from Base_Test now, and at the same time keeping in mind the fact that we will import and use evm-utils, and the file tests/utils/Utils.sol will be removed after that change

another idea came to mind: what if we move all these functions into Utils? IMO they are more of utility functions, especially the compute address functions and tranchesMerkleLT.

even though the current version works, just asking as i believe we can improve it (and sorry for so many changes :)) )

@smol-ninja
Copy link
Member Author

smol-ninja commented Feb 18, 2025

I didn't get what you meant in your first sentence. We will have to keep Base.t.sol anyhow because the setUp has some bespoke implementation such as merkleFactory.setMinimumFee(defaults.MINIMUM_FEE()).

Also, with respect to moving those function to Utils, you pointed to the previous file. After the PR, the Base contains few more functions as you can see here. Plus Base inherits from Constants as well.

Regardless, after evm-utils, we should still have Base file.

@andreivladbrg
Copy link
Member

andreivladbrg commented Feb 18, 2025

I didn't get what you meant in your first sentence. We will have to keep Base.t.sol anyhow because the setUp has some bespoke implementation such as merkleFactory.setMinimumFee(defaults.MINIMUM_FEE()).

i have not said anything about Base.t.sol, but Utils.sol, which has all these functions, that can be found in evm-utils, thus we will be removing it once we install that here. and now, with this idea in mind (the fact that we - in the future - we will not have a tests/utils/Utils.sol anymore) what if we now move these functions (also pointed out in above message) in tests/utils/Utils.sol?

you pointed to the previous file

sorry about that, edited the above message

lmk if i've made self clear now

@smol-ninja smol-ninja force-pushed the feat/avca branch 2 times, most recently from 8a9eb88 to 156b3b1 Compare February 18, 2025 13:21
@smol-ninja smol-ninja force-pushed the tests/default-contract branch from b9a2fce to 1a71030 Compare February 18, 2025 13:23
@smol-ninja
Copy link
Member Author

As discussed on Slack, lets leave those functions in Base for now and re-assess it in the evm-utils PR.

Base automatically changed from feat/avca to staging February 18, 2025 17:09
@smol-ninja smol-ninja force-pushed the tests/default-contract branch from aee38ca to 6f08ca5 Compare February 18, 2025 17:19
@smol-ninja smol-ninja changed the base branch from staging to refactor/multiple-factories February 19, 2025 22:03
@smol-ninja smol-ninja force-pushed the tests/default-contract branch from 6f08ca5 to 0b7a7cd Compare February 19, 2025 22:04
@smol-ninja
Copy link
Member Author

I have resolved the conflicts on this PR and changed the base branch to refactor/multiple-factories

Base automatically changed from refactor/multiple-factories to staging February 20, 2025 00:12
test: remove redundant functions from Defaults

test: remove Defaults contract

fix bugs
@smol-ninja smol-ninja force-pushed the tests/default-contract branch from 0b7a7cd to f3ff0ee Compare February 20, 2025 10:13
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

left some comments:

test: rename function params with "_"
@andreivladbrg
Copy link
Member

pushed a commit dddc3c7

lmk if you don't agree with the changes and have a strong opinion about the previous version. if you don't, feel free to mark the comments above as resolved. i find it better this way

@smol-ninja
Copy link
Member Author

gtg. Thanks for the feedback.

@smol-ninja smol-ninja merged commit 958ce06 into staging Feb 20, 2025
7 checks passed
@smol-ninja smol-ninja deleted the tests/default-contract branch February 20, 2025 15:52
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