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

refactor: remove broker #1166

Merged
merged 6 commits into from
Feb 6, 2025
Merged

refactor: remove broker #1166

merged 6 commits into from
Feb 6, 2025

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Feb 4, 2025

Closes #1157

I also decided to rename the totalAmount parameter to depositAmount, as there was no other amount in the create function. I’m also considering removing the CreateEventCommon struct, as I believe we will no longer encounter the "Stack too deep" error, the only problem is that it will cause more changes to the app. @smol-ninja wdyt?

I'm surprised by how much this change reduces the code! This would be advantageous for integrators as well.

@andreivladbrg andreivladbrg force-pushed the refactor/remove-broker branch from ad17297 to 081789a Compare February 4, 2025 15:33
test: deploy contracts in fork tests
@andreivladbrg andreivladbrg changed the base branch from main to staging February 4, 2025 17:25
@smol-ninja
Copy link
Member

I'm surprised by how much this change reduces the code

Yes I was not expecting it to reduce 800 lines. Clearly a good call to remove the broker fee.

I’m also considering removing the Lockup.CreateEventCommon struct

OK. Sounds good to me. I'd suggest to take the opinion of Razvan and Mircea on it though since it only affects the app.

@PaulRBerg
Copy link
Member

cc @sablier-labs/frontend

Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

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

Great work on the PR @andreivladbrg. I have pushed a small commit (d62a8d4) just some natspec fixes and moved Base_Test.setUp() under TODO tag to signal it clearly that it needs to be removed later.

If all looks good, you can go ahead and merge it.

Re CreateEventCommon, can we do it in another PR and as a separate issue for easier review?

I have also not reviewed benchmarks dir since it will be removed in the future (#1170). I hope thats OK.

@andreivladbrg
Copy link
Member Author

andreivladbrg commented Feb 6, 2025

Re CreateEventCommon, can we do it in another PR and as a separate issue for easier review?

sounds good

I have also not reviewed benchmarks dir since it will be removed in the future (#1170). I hope thats OK.

that's ok, just keep in mind when you work on the new repo that there were made some changes in this branch


btw, pushed a commit to crease the coverage, don't know how we have missed it until now

@andreivladbrg andreivladbrg merged commit 7725fde into staging Feb 6, 2025
9 checks passed
@andreivladbrg andreivladbrg deleted the refactor/remove-broker branch February 6, 2025 23:15
@smol-ninja
Copy link
Member

that's ok, just keep in mind when you work on the new repo that there were made some changes in this branch

Yes we both will be co-authors of those changes. Also, in the future PR, in case it requires making changes to the benchmark code, then lets just remove it (#1170).

btw, pushed a commit to crease the coverage, don't know how we have missed it until now

Good catch.

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.

Remove Broker functionality
3 participants