Deposit erc20 tokens #80
Conversation
This reverts commit 70f68ef.
Codecov Report
@@ Coverage Diff @@
## main #80 +/- ##
==========================================
- Coverage 73.22% 68.15% -5.08%
==========================================
Files 16 18 +2
Lines 2024 2179 +155
==========================================
+ Hits 1482 1485 +3
- Misses 542 694 +152
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| #[allow(clippy::expect_used)] | ||
| let contract_function = bridge_contract | ||
| .function("deposit") | ||
| .expect("failed to get deposit function parameters"); |
There was a problem hiding this comment.
We should avoid using expect here. The function already returns a Result so bubble up the error might be the best option
There was a problem hiding this comment.
I decided to use expect in this case, because the error state should be unreachable. That's why I consciously included #[allow(clippy::expect_used)].
In this case, bubbling up this error would force the function user to take into account an unreachable type of error.
There was a problem hiding this comment.
You're right in pointing out that the error seems to be unreachable, as the only possible way it could fail is if the function name isn't correct. From my perspective, it's a good idea to avoid using expect. The rationale behind this is that there might be potential for ethers to introduce a bug in the future connected to that specific function or something closely related, causing a panic on our end. Besides that the function is already returning a result so, from the user side, they will have to handle the error anyways.
There was a problem hiding this comment.
The rationale behind this is that there might be potential for ethers to introduce a bug in the future connected to that specific function or something closely related, causing a panic on our end
This should be covered on testing.
Besides that the function is already returning a result so, from the user side, they will have to handle the error anyways.
But this adds another possible error state, forcing the user to write a handler somewhere for an unreachable state.
I think that this piece of code should be refactored in the future by doing this in a completely different way.
There was a problem hiding this comment.
I believe that my code in this PR sucks. It was all done in a rush.
There was a problem hiding this comment.
Okay no worries, we'll keep the expects/unwraps in mind. We can merge and create an issue for address this in the future
There was a problem hiding this comment.
Created issue #103 to track this one
| #[allow(clippy::expect_used)] | ||
| contract_function | ||
| .encode_input(¶ms.into_tokens()) | ||
| .expect("failed to encode deposit function parameters") | ||
| .into() | ||
| }; |
There was a problem hiding this comment.
We should avoid using expect here. The function already returns a Result so bubble up the error might be the best option
There was a problem hiding this comment.
I decided to use expect in this case, because the error state should be unreachable. That's why I consciously included #[allow(clippy::expect_used)].
In this case, bubbling up this error would force the function user to take into account an unreachable type of error.
There was a problem hiding this comment.
The discussion moved to #80 (comment)
There was a problem hiding this comment.
Created issue #103 to track this one
- Why private?
- This functionality is out of scope for our SDK.
- It wasn't being used outside this module.
- I think that it is ok to go with the simplest solution right now. We
could refactor this to emit an error in the future if necessary.
- It makes sense to use `expect`/`unwrap` here because the
`include_str!` macro ensures that the ABI JSON string is present.
jpcenteno
force-pushed
the
deposit-erc20-tokens
branch
from
4022f0f to
392ef65
Compare
closes #65