Enable ETH fees

[andreivladbrg]

Depends on #1084

Closes #1076 and #1090

[smol-ninja]

Shouldn't this PR be independent of #1084? Both are separate issues. One is related to batch functions which is primarily create related whereas this one is about withdraw function.

It's a good practice to create PRs independent of each other. If one takes time or requires more changes after review, the other PR can be reviewed and merged independently.

[andreivladbrg]

Shouldn't this PR be independent of #1084? Both are separate issues. One is related to batch functions which is primarily create related whereas this one is about withdraw function.

not necessarily. it’s not about one issue being directly related to the other, but rather about anticipating potential conflicts to avoid time wasted resolving them. you can see the independent changes related to eth fees, as the base branch is refactor/include-batch-core. after merging that branch, the base branch for this PR will automatically switch to staging once that PR is merged.

we should merge PRs in chronological order

It's a good practice to create PRs independent of each other.

why? since all will eventually be merged into staging, it’s more about focusing on the changes specific to this issue, which is exactly what you see with the current base branch.

If one takes time or requires more changes after review, the other PR can be reviewed and merged independently.

since it's just the two of us working on these changes, and there aren't multiple contributors at the same time (where your approach would be preferred), i believe this approach is more efficient.
also, based on my experience with the unlock amount feature and the conflicts that arose, i can confidently say that creating PRs dependent on others is often more efficient.

scenario 1 (sequentially dependent PRs)

1. A <-- B <-- C <-- D
2.       B <-- B review
3.             C <-- rebase B review
4.             C <-- C review 
5.                   D <-- C rebase 
6.                   D <-- D review

scenario 2 (independent PRs with higher conflicts)

1. A <-- B 
2. A <-- C # changes similar files that B changes (conflicts warning)
3. A <-- D # changes similar files that B and C changes (conflicts warning)
4. B <-- B review
5. C <-- rebase B
7. D <-- rebase B
8. C <-- C review
9. D <-- rebase C
10. D <-- review D

as you can see, scenario 1 is better

[smol-ninja]

You made some good points. I gave it more thought, and I think what you said is accurate when the time complexity of reviewing, resolving comments, and merging all PRs is the same. In many cases, the time complexities are different. Making all PRs dependent on each other would mean it disallows us from reviewing, resolving, and merging them asynchronously. Even though some PRs may be reviewed and merged faster, they will continue to be blocked by the base PR, which may take more time than anticipated. Also, in the case of a chain of PRs, when changes are made to the base PR, that means making changes to all subsequent PRs.

Your point about rebasing is valid, but in my experience, we can identify such PRs/issues that are truly dependent on each other. In those cases, I agree with you. But generalizing it can slow us down in many cases.

As an example, I rebased #1091 from staging in less than 30 minutes, and it can now also be reviewed and merged independently, which otherwise could stay open for as long as the other PRs are open. Another example is that #1087 can be worked upon, reviewed and merged independently of other PRs opened before it.

From my experience, its easier to deal with independent PRs which I also prefer as much as possible, which you can review and merge independent of other PRs. This, imo, speeds up closing the issues faster without getting blocked by the base PR.

[andreivladbrg]

@smol-ninja i have marked the PR as ready to review, tho the fork tests are failing, we can ignore them for now, since we will remove the "periphery" contract in the upcoming PR

(will answer tomorrow to your reply)

[smol-ninja]

Have you also included #1090 in this PR as discussed here? If yes, can you please mention it in the description?

[andreivladbrg]

Have you also included #1090 in this PR as discussed here? If yes, can you please mention it in the description?

not yet, facing some issue with it, we might be able to implement it, due to requiring all functions within calls array to be payable as well

[andreivladbrg]

@smol-ninja Thank you for your detailed reply.

You made some good points. I gave it more thought, and I think what you said is accurate when the time complexity of reviewing, resolving comments, and merging all PRs is the same.

Indeed, complexity plays a significant role in this discussion.

In many cases, the time complexities are different

You are correct that PR complexities can vary. IMO, the sequential approach works best for handling highly complex PRs, whereas the parallel approach is more suited for small changes.

Making all PRs dependent on each other would mean it disallows us from reviewing

That's incorrect and I disagree with your assertion regarding the base branch of the PR. As I mentioned in my initial comment, what truly matters is the base branch the PR is raised against. For example, if I start from refactor/include-batch-core and create a PR with the base branch set as refactor/include-batch-core, you will only review the changes specific to feat/eth-fee. This is because the git diff will display only those changes.

Even though some PRs may be reviewed and merged faster, they will continue to be blocked by the base PR

I don’t quite understand why this point is relevant. Whether a PR is merged into staging after 1 day or after 5 days doesn’t seem to matter as long as all PRs ultimately need to be merged into staging for the code to be audited.

Let me give an example: Suppose everything must be merged into staging. I create branches A and B, both involving complex changes and targeting the same files and lines (guaranteeing conflicts). Branch B is created after Branch A, but the PR for Branch B is raised first. The review processes for both start simultaneously. If PR A takes 1 day for review and changes, and PR B takes 2 days, merging PR A immediately upon completion would require PR B to be rebased and conflicts resolved, adding another day (3 days total).

In contrast, if Branch B were created sequentially from Branch A, rebasing the review changes of PR A into Branch B would only take half a day. In this case, having both branches on staging would take 2.5 days instead of 3 days. For more complex PRs, I still believe the sequential method is more efficient.

Another argument is that, for us (contract development), we don’t need specific features merged into the main branch as soon as possible, unlike other repositories (like v2-interfaces). In that case, I completely agree with you that parallel PRs are the way to go. However, for us, since we release our code after it has been audited, we don’t need to merge into the main or staging branch immediately. I hope I have made myself clear

which may take more time than anticipated. Also, in the case of a chain of PRs, when changes are made to the base PR, that means making changes to all subsequent PRs.

As mentioned above, in the end, it might take more time to have all changes (what really matters) merged into staging

As an example, I rebased #1091 from staging in less than 30 minutes, and it can now also be reviewed and merged independently,

This is a small PR, and it’s understandable that it didn’t take much time (which I agree is not that relevant to handle in sequence). However, I come from the singleton and unlock amounts PR, where rebasing took a lot of time (1 day).

which otherwise could stay open for as long as the other PRs are open. Another example is that #1087 can be worked upon, reviewed and merged independently of other PRs opened before it.

IMO, as said above, that's not that relevant when we merge it if we don't have all changes ready for auditing. But I understand the argument since it is a small PR

From my experience, its easier to deal with independent PRs which I also prefer as much as possible, which you can review and merge independent of other PRs.

And from my experience, when it comes to more complex PRs, it’s generally easier to handle them sequentially.”

speeds up closing the issues faster without getting blocked by the base PR

not necessarily

---

PS: You don’t need to reply to this now. We already discussed on Slack using the parallel option

[andreivladbrg]

hmm, a weird error we are facing when compiling

not sure yet what causes it

[smol-ninja]

not sure yet what causes it

I will investigate. Feel free to go to bed :))

[smol-ninja]

Great work @andreivladbrg. Left some comments below. I have also included a commit similar to sablier-labs/airdrops@0611cf9 which renamed sablier fee to simply fee.

[PaulRBerg]

Closing in favor of #1093.