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

Fix the bug in deposit #151

Merged
merged 7 commits into from
Jun 4, 2024
Merged

Fix the bug in deposit #151

merged 7 commits into from
Jun 4, 2024

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Jun 3, 2024 •
edited
Loading

Linked issues

Changelog

Src

  • Fixes the bug by changing the "amount" paramater in deposit to transferAmount
  • Don't allow ERC20 assets with more than 18 decimals
  • Add Helpers library
  • Move _calculateTransferAmount and _safeAssetDecimals in Helpers
  • Add calculateNormalizedAmount function
  • Add checkAndCalculateBrokerFee function in Helpers and use it in _depositViaBroker

Test

  • Add new constants with 6 decimals
  • Update the tests with the new logic in deposit
  • Add more tests in create
  • Add more tests in deposit

Question: should we add independent tests for Helpers.calculateNormalizedAmount and Helpers.calculateTransferAmount?

@andreivladbrg
feat: add Helpers library
a9a5f7d 
feat: implement calculateNormalizedAmount function
refactor: move calculateTransferAmount in Helpers
refactor: rename all parameters in "deposit" function to
"transferAmount"
@andreivladbrg
test: remove no longer needed normalize functions
02a3c7a 
test: introduce new constants
test: add more tests for deposit function
test: update tests with new parameter name "transferAmount"
test: use Helpers library in tests
@andreivladbrg
feat: add checkAndCalculateBrokerFee function helpers
e3765f4 
refactor: use the new function in _depositViaBroker function
refactor: move _safeAssetDecimals in Helpers
chore: remove streamId from custom error
@andreivladbrg andreivladbrg requested a review from smol-ninja June 3, 2024 15:19
smol-ninja
smol-ninja requested changes Jun 3, 2024
View reviewed changes
Copy link
Member

@smol-ninja smol-ninja left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice PR @andreivladbrg. I briefly looked through it and I can see you have moved functions to Helpers contract which is awesome.

I ended up using your suggestion to not revert in calculateNormalizedAmount.

Thank you.

should we add independent tests for Helpers.calculateNormalizedAmount and Helpers.calculateTransferAmount

I don't think its necessary. A complete test suite for public functions should be sufficient to test these internal functions as well.

Also, I think we should start using unchecked wherever its safe to do so inspite of it reducing readability. We should save gas on mathematical operations.

PS: This was just a brief review. I will do a full review later.

src/libraries/Helpers.sol Outdated Show resolved Hide resolved
test/Base.t.sol Outdated Show resolved Hide resolved
@andreivladbrg
perf: use unchecked to optimize calculate functions
3cc9212 
test: rename asset with 24 decimals
@andreivladbrg

This comment was marked as outdated.

Sign in to view
@smol-ninja

This comment was marked as outdated.

Sign in to view
@andreivladbrg
fix: don't support asset with more than 18 decimals
85c6996 
refactor: update Helpers calculate functions
test: remove assetWith24Decimals from Base_Test
test: when the asset has more than 18 decimals in create
docs: update natspec in the interface
docs: update README
@andreivladbrg
Copy link
Member Author

@smol-ninja updated the PR with not allowing assets with more than 18 decimals

smol-ninja
smol-ninja requested changes Jun 4, 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.

Thanks for making the changes so quickly. Left few comments below.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/integration/concrete/withdraw-at/withdrawAt.t.sol Outdated Show resolved Hide resolved
test/invariant/Flow.t.sol Outdated Show resolved Hide resolved
test/utils/Constants.sol Outdated Show resolved Hide resolved
test/utils/Constants.sol Outdated Show resolved Hide resolved
test/utils/Constants.sol Outdated Show resolved Hide resolved
@andreivladbrg
test: rename constant
17c2235 
test: shorten variable names
docs: improve README
smol-ninja
smol-ninja approved these changes Jun 4, 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.

@andreivladbrg andreivladbrg merged commit 442b268 into main Jun 4, 2024
6 checks passed
@andreivladbrg andreivladbrg deleted the fix/rounding-bug branch June 4, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

Drop support for ERC20 tokens with decimals > 18 Bug in deposit if the asset decimals are less than 18
2 participants
@andreivladbrg @smol-ninja