yanue / starkex

stark key authentication library, signature generator for dydx exchange
MIT License
19 stars 10 forks source link

Concurrent version of PedersenHash isn't working #7

Closed emil14 closed 1 year ago

emil14 commented 1 year ago

Is this library still works? Could you please admit it? It looks like I didn't change anything in my code but it's not working any longer.

This is how I use it

    orderSignParam := starkex.OrderSignParam{
        NetworkId:  r.networkID, // 3
        PositionId: r.positionID, // got from getAccount()
        Market:     req.Market, // "ETH-USD"
        Side:       "BUY",
        HumanSize:  req.Size, // 1
        HumanPrice: req.Price, // 1
        LimitFee:   req.LimitFee, // "0.000000"
        ClientId:   req.ClientId, // "1" 
        Expiration: expiration,  // "2023-04-05T15:50:15.285Z"
    }

    signature, err := starkex.OrderSign(r.starkexCreds.PrivateKey, orderSignParam) // FIXME invaid signature
    if err != nil {
        return fmt.Errorf("starkex sing: %w", err)
    }

UPD: clientID if generated now, it wasn't the problem

I think you last commit where you've added concurrency to crypto stuff broke something. Rollback to the prev version fixed the problem

tpunt commented 1 year ago

At a quick glance, it looks as though the wg.Add(1) (on line 43) needs to happen outside of the goroutine creation in commit a654684 @yanue

tpunt commented 1 year ago

Actually, I don't see how making the PedersenHash function concurrent like this could work anyway...

Also, the extended Euclidean algorithm implementation (igcdex()) looks broken. E.g. the first if statement is comparing the same conditions.

I've forked the project for now (tpunt/starkex) to apply the fixes, since the owner is not currently around. Happy to create PRs to fix things in this repo when the owner is back (I really don't want to maintain a fork of this).

emil14 commented 1 year ago

Awesome! Thank you so much @tpunt