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.

Sign up for GitHub

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

Jump to bottom

test: refactor all tests according to singleton contracts #1077

Merged
merged 1 commit into from
Nov 13, 2024
Merged

test: refactor all tests according to singleton contracts #1077

merged 1 commit into from
Nov 13, 2024

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Nov 5, 2024 •
edited
Loading

This PR contains tests according to singleton contract. Upon review, this PR should be merged into #1069.

To do

  • Update Periphery tests
  • Update core integration tests
  • Update core fuzz tests
  • Update core invariant tests
  • DRY'ify shared tests: remove abstract from the contracts that do not depend on the lockup model
  • Update benchmarks
  • Fix precompiles to be compatible with Public libraries
  • Update markdowns
  • Write missing tests, check coverage
  • DRY'ify benchmarks
  • Refactor getters similar to flow #1081
  • Take the minimum of tranches count and segments count and update it

Resources:

  1. Loom video explaining changes in integration tests

@smol-ninja smol-ninja marked this pull request as draft November 5, 2024 18:31
@smol-ninja smol-ninja mentioned this pull request Nov 6, 2024
Merged
17 tasks
@smol-ninja smol-ninja force-pushed the test/singleton-contract branch from 15240b6 to 80dbae3 Compare November 6, 2024 15:45
@smol-ninja smol-ninja marked this pull request as ready for review November 7, 2024 17:44
@smol-ninja smol-ninja changed the base branch from refactor/singleton-contract to staging November 7, 2024 20:19
@smol-ninja smol-ninja changed the base branch from staging to refactor/singleton-contract November 7, 2024 20:19
@smol-ninja smol-ninja force-pushed the test/singleton-contract branch 2 times, most recently from 7d2dd69 to 8f58ce8 Compare November 8, 2024 00:36
@smol-ninja smol-ninja force-pushed the refactor/singleton-contract branch from 5634eff to b4d9ea6 Compare November 8, 2024 21:55
@smol-ninja smol-ninja mentioned this pull request Nov 8, 2024
Merged
@PaulRBerg PaulRBerg changed the title test: refactor all tests according to singelton contracts test: refactor all tests according to singleton contracts Nov 9, 2024
@smol-ninja
Copy link
Member Author

@andreivladbrg, what do you think about prefixing error names with Helpers in Helpers contract. When an error is thrown by the Helpers contract, the user wont know which contract threw it. Also, its not following the naming convention <CONTRACT_NAME>:<ERROR_NAME>

Any feedback on Loom recording? Was it more helpful compared to if I would have written explanations in the PR description?

@PaulRBerg
Copy link
Member

what do you think about prefixing error names with Helpers in Helpers contract

What about SablierHelper? The point of named errors is for them to be easy to identify up the revert stack.

@smol-ninja
Copy link
Member Author

What about SablierHelper? The point of named errors is for them to be easy to identify up the revert stack

The errors would only be thrown when a tx with SablierLockup as the to address reverts. So, is that not enough to identify up the revert stack?

  • to is SablierLockup contract
  • Error name is Helpers_<something>.

We can change it to SablierHelper as well. But since its only used in Lockup, LockupHelper would be more apt.

@PaulRBerg
Copy link
Member

Other contracts can call the SablierLockup contract.

I am strongly in favor of using SablierHelpers or SablierLockupHelpers as a prefix.

@smol-ninja
Copy link
Member Author

smol-ninja commented Nov 10, 2024 •
edited
Loading

I am in for SablierHelpers then. @andreivladbrg I have added it as a task in src PR description.

@andreivladbrg andreivladbrg force-pushed the refactor/singleton-contract branch from 151dbdd to 57d7def Compare November 11, 2024 09:16
@andreivladbrg andreivladbrg force-pushed the test/singleton-contract branch from 358a061 to a06b1f2 Compare November 11, 2024 09:53
@smol-ninja smol-ninja force-pushed the refactor/singleton-contract branch from 57d7def to 8ca1274 Compare November 11, 2024 10:01
@smol-ninja smol-ninja force-pushed the test/singleton-contract branch 2 times, most recently from 80099e1 to 914f13e Compare November 11, 2024 10:29
@smol-ninja smol-ninja force-pushed the refactor/singleton-contract branch from 8ca1274 to 9050f98 Compare November 11, 2024 10:33
@andreivladbrg andreivladbrg force-pushed the test/singleton-contract branch from 914f13e to 0828db8 Compare November 11, 2024 22:57
@andreivladbrg andreivladbrg mentioned this pull request Nov 12, 2024
Merged
andreivladbrg
andreivladbrg requested changes Nov 12, 2024
View reviewed changes
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.

It turned out to be a bigger PR than expected, shesh :)). Fast work!
I have some recommendations, along with the ones we already talked about on Slack - they are mostly addressed in the PR raised on top of this

package.json Show resolved Hide resolved
test/core/integration/concrete/lockup-tranched/get-tranches/getTranches.tree Show resolved Hide resolved
test/core/integration/concrete/lockup-dynamic/get-segments/getSegments.tree Show resolved Hide resolved
benchmark/EstimateMaxCount.t.sol
@@ -65,7 +65,7 @@ contract EstimateMaxCount is Test {

/// @notice Estimate the maximum number of tranches allowed in LockupTranched.
function test_EstimateTranches() public {
Copy link
Member

Choose a reason for hiding this comment

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

let's remove this, as we use only the segments count

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd preferred to keep it because right now, even though the segments count is less than tranches count but there could be a case in the future where tranches count may exceed segments count. So instead of rewriting it again, I'd leave the implementation.

A better approach would be to take the minimum of tranches count and segments count and update it, which is also something I intent to do (added to the OP).

Copy link
Member

Choose a reason for hiding this comment

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

could be a case in the future where tranches count may exceed segments count

how exactly? a tranche has the same variables as the segments expect for the exponent

A better approach would be to take the minimum of tranches count and segments count and update it

thought about it but i find it unneeded

Copy link
Member Author

Choose a reason for hiding this comment

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

a tranche has the same variables as the segments expect for the exponent

Yes but the computation part can differ. If you are confident that segments will always be more gas expensive then we can remove it. I am doubtful so I thought the minimum of the two would be a safer choice. I leave it for you to decide.

script/core/Init.s.sol Outdated Show resolved Hide resolved
test/core/integration/fuzz/lockup-base/cancelMultiple.t.sol Show resolved Hide resolved
test/core/integration/fuzz/lockup-base/withdrawMaxAndTransfer.t.sol Show resolved Hide resolved
test/core/integration/fuzz/lockup-base/withdrawMultiple.t.sol Show resolved Hide resolved
test/core/invariant/handlers/BaseHandler.sol Outdated Show resolved Hide resolved
test/core/invariant/handlers/LockupCreateHandler.sol Show resolved Hide resolved
Base automatically changed from refactor/singleton-contract to staging November 13, 2024 22:11
@smol-ninja @andreivladbrg
test: refactor all tests according to singleton contracts
86adfca 
test: update periphery tests

chore: bump version to v1.3.0

 This is a combination of 14 commits.

test: update tests accordingly to singletone architecture

test: update fork tests for core

test(core): refactor integration tests for core

test(integration): rename Lockup_Create_Integration_Shared_Test to Lockup_Integration_Shared_Test

test: dry'ify concrete tests

test(core): update fuzz tests

test(core): updates invariant tests

test(benchmark): update benchmark

test: fix file path

build: link libraries to optimized build

test: move nullStreamId variable into Integration tests
build: etch helpers bytecode onto dummy addresses for optimized tests
test: update precompiles

test: fix benchmarks

test: polish librariy linkage

test: mock lockup in periphery fork test

test: deploy libraries through DeployOptimized contract
@andreivladbrg andreivladbrg force-pushed the test/singleton-contract branch from 0828db8 to 86adfca Compare November 13, 2024 22:14
@andreivladbrg andreivladbrg merged commit d318e99 into staging Nov 13, 2024
2 of 3 checks passed
@andreivladbrg andreivladbrg deleted the test/singleton-contract branch November 13, 2024 22:15
andreivladbrg added a commit that referenced this pull request Nov 13, 2024
@smol-ninja @andreivladbrg
test: refactor all tests according to singleton contracts (#1077)
9b4e320 
test: update periphery tests

chore: bump version to v1.3.0

 This is a combination of 14 commits.

test: update tests accordingly to singletone architecture

test: update fork tests for core

test(core): refactor integration tests for core

test(integration): rename Lockup_Create_Integration_Shared_Test to Lockup_Integration_Shared_Test

test: dry'ify concrete tests

test(core): update fuzz tests

test(core): updates invariant tests

test(benchmark): update benchmark

test: fix file path

build: link libraries to optimized build

test: move nullStreamId variable into Integration tests
build: etch helpers bytecode onto dummy addresses for optimized tests
test: update precompiles

test: fix benchmarks

test: polish librariy linkage

test: mock lockup in periphery fork test

test: deploy libraries through DeployOptimized contract

Co-authored-by: andreivladbrg <[email protected]>
andreivladbrg added a commit that referenced this pull request Jan 27, 2025
@smol-ninja @andreivladbrg
test: refactor all tests according to singleton contracts (#1077)
fd20480 
test: update periphery tests

chore: bump version to v1.3.0

 This is a combination of 14 commits.

test: update tests accordingly to singletone architecture

test: update fork tests for core

test(core): refactor integration tests for core

test(integration): rename Lockup_Create_Integration_Shared_Test to Lockup_Integration_Shared_Test

test: dry'ify concrete tests

test(core): update fuzz tests

test(core): updates invariant tests

test(benchmark): update benchmark

test: fix file path

build: link libraries to optimized build

test: move nullStreamId variable into Integration tests
build: etch helpers bytecode onto dummy addresses for optimized tests
test: update precompiles

test: fix benchmarks

test: polish librariy linkage

test: mock lockup in periphery fork test

test: deploy libraries through DeployOptimized contract

Co-authored-by: andreivladbrg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreivladbrg andreivladbrg andreivladbrg requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smol-ninja @PaulRBerg @andreivladbrg