erc20<->cosmos coin amount conversion #19

sangier · 2024-08-06T12:18:37Z

Implements a library for the conversion of ERC20 amounts to cosmos coin amounts.

closes #5

src/utils/ERC20CosmosCoinAmountConversion.sol

gjermundgaraba · 2024-08-06T13:35:26Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+    }
+}
+
+contract ERC20CosmosCoinAmountConversionTest is Test {


Might be worth using both fuzz and invariants testing here: https://book.getfoundry.sh/forge/advanced-testing

It's absolutely a great Idea! Only wonder, do we want to address this now? or later on? If you guys needs this lib asap to complete your workflows, I can address this in a later on PR and refactor the testing file. My primary goal was to ensure it works properly :)

Let's address this now. We don't require this lib asap since our mock ERC20's size doesn't cause issues. So it is ok to take time to make sure we design and test this properly.

gjermundgaraba · 2024-08-06T13:39:46Z

src/utils/ERC20CosmosCoinAmountConversion.sol

+        uint256 factor;
+        // Case ERC20 decimals are bigger than cosmos decimals
+        if (tokenDecimals > DEFAULT_COSMOS_DECIMALS) {
+            factor = 10 ** (tokenDecimals - DEFAULT_COSMOS_DECIMALS);


Are there any scenarios where this can cause any overflows?

Solidity version starting from 0.8 supports built-in underflows and overflows checks for arithmetic operations with automatic reverting. We should be good!

gjermundgaraba · 2024-08-06T13:40:04Z

src/utils/ERC20CosmosCoinAmountConversion.sol

+            // Case ERC20 decimals < DEFAULT_COSMOS_DECIMALS
+            // Note we need to amplify the decimals
+            factor = 10 ** (DEFAULT_COSMOS_DECIMALS - tokenDecimals);
+            temp_convertedAmount = amount * factor;


Same here, could there be an overflow here?

gjermundgaraba · 2024-08-06T13:42:38Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+import { ERC20CosmosCoinAmountConversion } from "../src/utils/ERC20CosmosCoinAmountConversion.sol";
+import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+// Discuss - Do we want to move mock contract to a mock folder?


There are already some mocks, might make sense to keep mocks separate, yes.
Maybe we can also reuse the ERC20 mocks that exist there to some extent also.

Yes, please move or reuse the other ERC20 mock if possible.

gjermundgaraba · 2024-08-06T13:45:16Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+     * - Converted Amount: 1000000000000000001 / 10^12 = 1000000 (Cosmos coins).
+     * - Remainder: 1000000000000000001 % 10^12 = 1 (Wei remaining).
+     */
+    function testConvertMockERC20TokenAmountToCosmosCoin_1() public view {


It might change a bit if you decide upon using the fuzzing framework as many of these tests could be written as a single test, but just a general comment: I personally prefer the test functions to be more clearly named what scenario/sub-scenario they are testing, rather than 1, 2, 3 etc.

Definitely agree. Reasons for this naming are here

Yes, please rename the tests. It'd be good to refactor these to use table test pattern if possible. You can see me do this here.

Does the fuzz tests with fixtures do the same @srdtrk?

https://book.getfoundry.sh/forge/fuzz-testing#fuzz-test-fixtures

gjermundgaraba · 2024-08-06T13:47:08Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+        assertEq(convertedAmount, expectedAmount, "Conversion mismatch for higher decimals");
+    }
+
+    function testConvertCosmosCoinAmountToERC20_LessThanSixDecimals() public {


I think it would be good to try to cover a few more failure scenarios: ideally triggering all the reverts we have if possible :)

Addressed here

I think we can add more failure scenarios. But we don't need to hit everything as long as we add a TODO for this and address it in the future

Co-authored-by: Gjermund Garaba <[email protected]>

srdtrk

Looks amazing @sangier, thanks for the superb work. And don't worry, we have enough time to address the PR reviews since this is not required for e2es to pass or be written.

This PR would be needed to deploy this to a testnet and use WETH.

srdtrk · 2024-08-07T05:45:08Z

src/utils/ERC20CosmosCoinAmountConversion.sol

+    function _getERC20TokenDecimals(address tokenAddress) internal view returns (uint8) {
+        // Input validation
+        // TODO Add custom error?
+        require(tokenAddress != address(0), "Address cannot be the zero address");


Introduce custom errors. For now, we will use reverts but we can change to require in the next version of solidity. Do this for all require please

srdtrk · 2024-08-07T05:45:54Z

src/utils/ERC20CosmosCoinAmountConversion.sol

+        require(tokenAddress != address(0), "Address cannot be the zero address");
+        // TODO Reason about this check - Remember it as a good practice but what real protections give us?
+        // TODO Add custom error?
+        require(tokenAddress != address(this), "Address cannot be the contract itself");


probably we can remove this for now

srdtrk · 2024-08-07T05:49:09Z

src/utils/ERC20CosmosCoinAmountConversion.sol

+        // Note if we want to allow different cosmos decimals, we need to handle that here too
+        // Note that tokenAddress input validations are executed in the _getERC20TokenDecimals function
+        uint8 tokenDecimals = _getERC20TokenDecimals(tokenAddress);
+        // TODO write an ADR for this?


Repo is not mature enough for ADRs yet. So we can remove this.

src/utils/ERC20CosmosCoinAmountConversion.sol

srdtrk · 2024-08-07T06:10:50Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+import { ERC20CosmosCoinAmountConversion } from "../src/utils/ERC20CosmosCoinAmountConversion.sol";
+import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+// Discuss - Do we want to move mock contract to a mock folder?


Yes, please move or reuse the other ERC20 mock if possible.

srdtrk · 2024-08-07T06:12:43Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+    }
+}
+
+contract ERC20CosmosCoinAmountConversionTest is Test {


Let's address this now. We don't require this lib asap since our mock ERC20's size doesn't cause issues. So it is ok to take time to make sure we design and test this properly.

srdtrk · 2024-08-07T06:16:00Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+     * - Converted Amount: 1000000000000000001 / 10^12 = 1000000 (Cosmos coins).
+     * - Remainder: 1000000000000000001 % 10^12 = 1 (Wei remaining).
+     */
+    function testConvertMockERC20TokenAmountToCosmosCoin_1() public view {


Yes, please rename the tests. It'd be good to refactor these to use table test pattern if possible. You can see me do this here.

srdtrk · 2024-08-07T06:17:30Z

test/ERC20CosmosCoinAmountConversionTest.t.sol

+        assertEq(convertedAmount, expectedAmount, "Conversion mismatch for higher decimals");
+    }
+
+    function testConvertCosmosCoinAmountToERC20_LessThanSixDecimals() public {


I think we can add more failure scenarios. But we don't need to hit everything as long as we add a TODO for this and address it in the future

test/ERC20CosmosCoinAmountConversionTest.t.sol

Co-authored-by: srdtrk <[email protected]>

sangier · 2024-08-07T09:55:27Z

src/errors/IIErrors.sol

+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.25;
+
+interface IIErrors {


Created this file to manage errors that are common for all the contracts. Is the name ok or shall we call it IICommonErrors?

I don't know if we need this yet, I'd prefer not to have it yet. So maybe just use errors specific to your library

srdtrk

Looks good. Please address my comments, and then we can merge this

srdtrk · 2024-08-07T10:09:21Z

test/sdkCoinTest.t.sol

Rename this file to use capital letters SdkCoinTest.t.sol

src/utils/SdkCoin.sol

srdtrk · 2024-08-07T10:14:13Z

src/utils/SdkCoin.sol

+import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";
+import { IISdkCoinErrors } from "../errors/IISdkCoinErrors.sol";
+
+library SdkCoin {


Suggested change

library SdkCoin {

library SdkCoin is IISdkCoinErrors {

Would be great but seems this is not possible for libraries!

src/utils/SdkCoin.sol

Co-authored-by: srdtrk <[email protected]>

sangier added 11 commits August 2, 2024 16:12

add base-wip

a7f774c

fixes

8016b6d

add test

9d90acf

fixes

22f78b3

add support for flexible ERC20 decimals

2212ed7

add erc20 amount conversion tests

a5950b6

test fixes, add cosmosCointoERC20 conversion

38124e3

add tests

559d3cd

fixes

d7f61a3

rm leftover

eab745f

lint

72ee601

gjermundgaraba reviewed Aug 6, 2024

View reviewed changes

sangier and others added 3 commits August 6, 2024 16:12

Apply suggestions from code review

7d01d34

Co-authored-by: Gjermund Garaba <[email protected]>

add tests for revert scenarios

785a18c

function parameters refactor for consistency

98c76fe

srdtrk reviewed Aug 7, 2024

View reviewed changes

sangier and others added 4 commits August 7, 2024 10:05

Apply suggestions from code review

71e78fc

Co-authored-by: srdtrk <[email protected]>

support for review suggestions

f087e77

custom errors, fix function signatures

f5526d2

upper case fix

ba85ba5

sangier commented Aug 7, 2024

View reviewed changes

rm common errors

7502e67

srdtrk approved these changes Aug 7, 2024

View reviewed changes

sangier and others added 3 commits August 7, 2024 12:22

Apply suggestions from code review

cddee60

Co-authored-by: srdtrk <[email protected]>

address review

4c22c5c

rename file

cd6e1cc

srdtrk merged commit cf5d185 into main Aug 7, 2024
7 checks passed

srdtrk deleted the stefano/erc20-coin-conversion branch August 7, 2024 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

erc20<->cosmos coin amount conversion #19

erc20<->cosmos coin amount conversion #19

sangier commented Aug 6, 2024

gjermundgaraba Aug 6, 2024

sangier Aug 6, 2024 •

edited

Loading

srdtrk Aug 7, 2024

gjermundgaraba Aug 6, 2024

sangier Aug 6, 2024 •

edited

Loading

gjermundgaraba Aug 6, 2024

sangier Aug 6, 2024

gjermundgaraba Aug 6, 2024

srdtrk Aug 7, 2024

gjermundgaraba Aug 6, 2024

sangier Aug 6, 2024

srdtrk Aug 7, 2024

gjermundgaraba Aug 7, 2024

gjermundgaraba Aug 6, 2024

sangier Aug 6, 2024

srdtrk Aug 7, 2024

srdtrk left a comment

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

sangier Aug 7, 2024

srdtrk Aug 7, 2024

srdtrk left a comment

srdtrk Aug 7, 2024

srdtrk Aug 7, 2024

sangier Aug 7, 2024

erc20<->cosmos coin amount conversion #19

erc20<->cosmos coin amount conversion #19

Conversation

sangier commented Aug 6, 2024

Choose a reason for hiding this comment

sangier Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangier Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangier Aug 6, 2024 •

edited

Loading

sangier Aug 6, 2024 •

edited

Loading