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

feat: add eth fees in flow functions #348

Merged
merged 4 commits into from
Dec 16, 2024
Merged

feat: add eth fees in flow functions #348

merged 4 commits into from
Dec 16, 2024

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Dec 13, 2024 •
edited
Loading

Closes #328

Note: Added concrete tests for this to ensure that we don't miss marking a function as payable

@andreivladbrg
feat: add eth fees in flow functions
4794aa4 
test: update tests accordingly
PaulRBerg
PaulRBerg requested changes Dec 13, 2024
View reviewed changes
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.

Thanks Andrei.

Good idea to write concrete tests, but it's overkill to test these things:

  • it should update the caller ETH balance
  • it should update the flow contract ETH balance

Just having the call compiling with the { value } option would be enough. We are not meant to test that the EVM is implemented correctly.

So let's please simplify those tests.

smol-ninja
smol-ninja requested changes Dec 14, 2024
View reviewed changes
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.

This PR should be created against a new branch called staging.

tests/fork/Flow.t.sol Outdated Show resolved Hide resolved
smol-ninja
smol-ninja requested changes Dec 14, 2024
View reviewed changes
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.

Sorry, I accidentally clicked submit review. Here is full review.

tests/fork/Fork.t.sol Outdated Show resolved Hide resolved
tests/integration/concrete/collect-fees/collectFees.tree Outdated Show resolved Hide resolved
tests/integration/concrete/collect-fees/collectFees.t.sol Outdated Show resolved Hide resolved
tests/integration/concrete/collect-fees/collectFees.t.sol Outdated Show resolved Hide resolved
tests/integration/concrete/payable/payable.t.sol Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Dec 14, 2024 •
edited
Loading

Good idea to write concrete tests, but it's overkill to test these things:
it should update the caller ETH balance
it should update the flow contract ETH balance
Just having the call compiling with the { value } option would be enough. We are not meant to test that the EVM is implemented correctly.

thanks. fair enough, let's remove them. also, does it mean we should remove the address.balance check from all places?

@andreivladbrg andreivladbrg changed the base branch from main to staging December 14, 2024 18:18
@andreivladbrg andreivladbrg force-pushed the feat/eth-fees branch 3 times, most recently from 21ac526 to 18aec2c Compare December 14, 2024 18:39
@andreivladbrg
Copy link
Member Author

@PaulRBerg @smol-ninja updated the branch with your feedback, lmk if it looks good

smol-ninja
smol-ninja approved these changes Dec 15, 2024
View reviewed changes
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.

LGTM.

@smol-ninja smol-ninja merged commit 1df87d7 into staging Dec 16, 2024
7 checks passed
@smol-ninja smol-ninja deleted the feat/eth-fees branch December 16, 2024 04:44
This was referenced Dec 16, 2024
Merged
Merged
Merged
andreivladbrg added a commit that referenced this pull request Jan 27, 2025
@andreivladbrg @smol-ninja
feat: add eth fees in flow functions (#348)
282aacf 
* feat: add eth fees in flow functions

test: update tests accordingly

* test: add concrete tests for payable functions

* address feedback

* chore: remove redundant function

Signed-off-by: smol-ninja <[email protected]>

---------

Signed-off-by: smol-ninja <[email protected]>
Co-authored-by: smol-ninja <[email protected]>
andreivladbrg added a commit that referenced this pull request Jan 27, 2025
@andreivladbrg @smol-ninja
feat: add eth fees in flow functions (#348)
6b9e40b 
* feat: add eth fees in flow functions

test: update tests accordingly

* test: add concrete tests for payable functions

* address feedback

* chore: remove redundant function

Signed-off-by: smol-ninja <[email protected]>

---------

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

@PaulRBerg PaulRBerg PaulRBerg requested changes

@smol-ninja smol-ninja smol-ninja approved these changes

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

Successfully merging this pull request may close these issues.

ETH fees in withdraw function
3 participants
@andreivladbrg @smol-ninja @PaulRBerg