Infinite recursion warning #667

[albertopeam]

I have read the issue and it seems that the function in Utilities line 149 was commented but the code inside it was not copied to the function that now causes the recursion

Solution: I only forwarded the call from formatToEthereumUnits to formatToPrecision, which was the same as before introducing the recursion.

Could you take a look? I ran the unit tests but they crashed as it seems that I need a node running locally and I didn't read the doc yet. If you forward me to the doc I will install the env and I will ran the tests properly before submit the PR

[albertopeam]

nevermind, I found the doc https://github.com/web3swift-team/web3swift#development

[yaroslavyaroslav]

@albertopeam just started the ci/cd pipeline that will check it also

[albertopeam]

I have checked locally and the local test suite runs(I also saw the CI @yaroslavyaroslav, thanks for sharing), unfortunately this function that triggers the recursion doesn't have an associated test.
My intention was to create a test to demonstrate the fix is working but I have seen that the function is internal and no caller exists in the project right now so, I have a question, is needed this function? should I change to public func? I would say that it can be removed unless some future plan to use it

function signature

static func formatToEthereumUnits(_ bigNumber: BigInt, toUnits: Utilities.Units = .eth, decimals: Int = 4, decimalSeparator: String = ".") -> String?

[yaroslavyaroslav]

I think that we've discussion within Discord just right about that. @janndriessen @JeneaVranceanu folks are more involved in this one, so I guess they could say more.

[JeneaVranceanu]

@albertopeam
Thanks for taking a look at this issue.
Here are a few responses to your questions:

should I change to public func?

No, as that is not a scope of this issue and there were no requests to make it public so let's leave it internal.

I have seen that the function is internal and no caller exists in the project right now so, I have a question, is needed this function?

Looks like it indeed can be dropped as eventually the formatToPrecision is called and the main and only difference between formatToPrecision and formatToEthereumUnits looks to be the way the number of desired decimals is declared: numberDecimals: Int and toUnits: Utilities.Units respectively.

But I'd like to keep the toUnits: Utilities.Units because not all people know that ETH is divisible up to 18 decimal places. Though, that is something people learn quite fast.
So what I suggest is update the Units enumeration to the following representation:

extension Utilities {
    /// Various units used in Ethereum ecosystem
    public enum Units {
        case eth
        case wei
        case Kwei
        case Mwei
        case Gwei
        case Microether
        case Finney
        case custom(Int)

        public var decimals: Int {
            get {
                switch self {
                case .eth:
                    return 18
                case .wei:
                    return 0
                case .Kwei:
                    return 3
                case .Mwei:
                    return 6
                case .Gwei:
                    return 9
                case .Microether:
                    return 12
                case .Finney:
                    return 15
                case .custom(let value):
                    return value
                }
            }
        }
    }
}

Update: I'd even use this example and update names + add new cases -> https://github.com/ethereum/eth-utils/blob/master/eth_utils/units.py

[albertopeam]

I have updated the PR with the comments received, if I misunderstood something feel free to comment :)
@JeneaVranceanu @yaroslavyaroslav, could you take a look?

[yaroslavyaroslav]

I'm just really not into this. But folks have both expertise and rights to handle that appropriately.

[janndriessen]

LGTM! Thanks for the changes. 🚀

[JeneaVranceanu]

Waiting for CI and merging.

[JeneaVranceanu]

@janndriessen I pushed a few commits to this branch. Please, take a look at the as they are small and re-approve this PR if you have no questions.
Thanks!

[janndriessen]

@janndriessen I pushed a few commits to this branch. Please, take a look at the as they are small and re-approve this PR if you have no questions. Thanks!

good catch with the type!

[janndriessen]

🚢 it.