-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: the calculation of the security deposit amount #436
fix: the calculation of the security deposit amount #436
Conversation
…lockchain' into 277-get-pen-for-gas-using-any-token-from-the-pendulum-network
…-pendulum-network
… errors about doubles polyfills definitions
…-pendulum-network
…-pendulum-network
…-pendulum-network
✅ Deploy Preview for rococo-souffle-a625f5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@Sharqiewicz I changed the calculation of the griefing collateral to be a hook instead of a simple function. This makes testing more complex so for simplicity I just removed the test cases you added 😬 I think mocking the hooks could be a little too much. WDYT? Besides that, I fixed all the remaining issue and this PR is ready for a final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebma Very good changes, very good collaboration on this task. I added 2 comments on what we could improve. As for testing, I think we could test the hook using @testing-library/react-hooks
I tried adding a test with that but I wasn't able to make this run unfortunately. Some things I learned:
I did some googling but the things mentioned didn't help. For example, this comment suggests babel-jest. I saw that we already use that however, see here and here. @Sharqiewicz any ideas? |
@ebmai have implemented hook tests using The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for fixing the test setup 🙏
Should we remove the dependencies to the pract-testing library then, if we don't need or use it?
Also, one thing that is unfortunate about the tests. Looking at the 'should calculate griefing collateral correctly' test case: It has the same structure as all the failing tests, except that we have another call with waitFor()
to wait for the useEffect
hook to fire. While this test is great, this means that our other test cases for the invalid value are not because if
expect(result.current.eq(new Big(0))).toBeTruthy();
Always works, no matter if it's the happy or unhappy path, then this condition is not enough. Maybe we can add a timeout and check that this value doesn't change within like 1 second? Or do you have another idea @Sharqiewicz?
src/hooks/spacewalk/__tests__/useCalculateGriefingCollateral.test.ts
Outdated
Show resolved
Hide resolved
@pendulum-chain/product please review this |
@ebma I tested the following scenario: |
The calculation of the security deposit is also accurate 43 PEN at the current value equals 2.5 Euros. Looks good to be released @ebma @Sharqiewicz |
@Sharqiewicz I improved the test now, please have another look and see if it looks okay to you. Then we merge 👍 Thanks for the review @vadaynujra |
@ebma Great changes. I approve ✅ |
What:
fix the calculation of the security deposit amount
How:
💥 I started from GET PEN branch #413 because there it includes refactored usePriceFetcher
✅ 🦖 Implement calculateGriefingCollateral for FeeBox > Security Deposit
✅ 🧪 Implement tests for calculateGriefingCollateral
Comments:
Closes: #416