Skip to content
This repository has been archived by the owner on Jan 31, 2025. It is now read-only.

test: add ASSET variable in Fork.t.sol to store forked asset #287

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Feb 7, 2024

closes #286

Summary

  • Rename asset to dai when referring to the default test asset.
  • Refactoring related to tests.

PaulRBerg
PaulRBerg previously approved these changes Feb 7, 2024
Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

looks fine

test/Base.t.sol Outdated Show resolved Hide resolved
test/fork/Fork.t.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
test/fork/Fork.t.sol Outdated Show resolved Hide resolved
test/fork/Fork.t.sol Outdated Show resolved Hide resolved
test/fork/batch/createWithMilestones.t.sol Outdated Show resolved Hide resolved
test/fork/batch/createWithRange.t.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
@andreivladbrg
Copy link
Member

Note: this PR may cause a lot of conflicts on staging.

Should we merge this into staging?

@smol-ninja
Copy link
Member Author

Note: this PR may cause a lot of conflicts on staging.
Should we merge this into staging?

I agree. If it's not a very critical issue (which I think it's not), even my opinion is to merge this PR against the staging branch. Do we all agree then?

@andreivladbrg
Copy link
Member

Do we all agree then?

I personally agree

@smol-ninja smol-ninja force-pushed the fix/asset-setup-fork-tests branch from de1ac19 to 79c1f26 Compare February 11, 2024 00:16
@smol-ninja smol-ninja changed the base branch from main to staging February 11, 2024 00:17
@smol-ninja
Copy link
Member Author

I've updated this against staging branch. This PR may be merged independently from other PRs.

@smol-ninja
Copy link
Member Author

@andreivladbrg do you want to review it once more so that I can merge it into staging? It can be merged independently and I will update other PRs according to the changes introduced by this.

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.

Looks good now

test/fork/assets/USDT.t.sol Show resolved Hide resolved
foundry.toml Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
@andreivladbrg andreivladbrg force-pushed the fix/asset-setup-fork-tests branch 2 times, most recently from 071e97e to 87812a7 Compare February 13, 2024 16:46
andreivladbrg and others added 2 commits February 13, 2024 19:01
refactor: name default test asset to "dai"
test: use low-level call to approve contract via approveContract()
doc: add dev comment to USDC.t.sol and USDT.t.sol
build: include ffi config in default foundry profile
@andreivladbrg andreivladbrg force-pushed the fix/asset-setup-fork-tests branch from 87812a7 to 17ec78b Compare February 13, 2024 17:01
@smol-ninja smol-ninja merged commit 241b49c into staging Feb 14, 2024
6 of 7 checks passed
@smol-ninja smol-ninja deleted the fix/asset-setup-fork-tests branch February 14, 2024 10:17
andreivladbrg pushed a commit that referenced this pull request Feb 15, 2024
* test: add "ASSET" variable in Fork.t.sol to store forked asset
refactor: name default test asset to "dai"
test: use low-level call to approve contract via approveContract()
doc: add dev comment to USDC.t.sol and USDT.t.sole

* test: define a computeMerkleLockupLLAddress without asset_ parameter
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forked asset is overridden by Base_Test.setUp() during fork tests
3 participants