server/asset/eth: refactor coins #1397
Conversation
buck54321
commented
Jan 8, 2022
buck54321
commented
buck54321
force-pushed
the
erc20-coiner-refactor
branch
from
40b0d90 to
699e2d1
Compare
dex/networks/eth/params.go
Outdated
| vGwei := new(big.Int).Div(v, BigGweiFactor) | ||
| if vGwei.IsUint64() { | ||
| return vGwei.Uint64() | ||
| } |
There was a problem hiding this comment.
We have a lot of redundancy with these helpers.
server/asset/eth/common.go hasnever mind this one, I was looking at an old commit.ToGweihelpers that can be removed. Also that one is bizarrely used in client (ExchangeWallet)- dex/networks/eth/common.co has
ToGweiandToWei
Also, the docs on this function say GweiToWei instead of WeiToGwei.
There was a problem hiding this comment.
Two things I was trying to avoid with the ToGwei.
- A package level BigGweiFactor. This is mutable. Is this safe? For one a consumer could accidentally mutate it.
bigisn't the easiest package to use imo. Whenever you havea := b.Add(c, d )you might not expect b to change here, because we just want to add c and d, but b is set to those and returned as a. Also, could an asset that is not one of ours be added that maliciously changes this? I'm not sure if that is something we are worried about, but imo starting with a constant, or at least not exporting it, is safer.
As an example, add this test to the top of eth_test.go and another test will fail:
func TestMutate(t *testing.T) {
fmt.Println("**************")
fmt.Println(dexeth.BigGweiFactor)
c := big.NewInt(2)
d := big.NewInt(3)
a := dexeth.BigGweiFactor.Add(c, d)
fmt.Printf("%p %p\n", a, dexeth.BigGweiFactor)
fmt.Println(dexeth.BigGweiFactor)
}
- A
big.Intmay not fit into auint64. It could be too large or negative. It would be unexpected, but have untested consequences. Should we at least panic?
There was a problem hiding this comment.
Good call on the BigGweiFactor.
- A
big.Intmay not fit into auint64.
That's the purpose of both ToGwei and the new IsUnit64 check in WeiToGwei. Which brings us back to @chappjc's original observation
We have a lot of redundancy with these helpers.
I have no problem having both ToGwei and WeiToGwei, the choice of which depends on if we want 1) to explicitly check for overflow/sign errors, or 2) just fall back to zero when not suitable for uint64. WeiToGwei clearly has some syntactic advantage, but whether it's appropriate needs to be considered in context.
I do think we should rename ToGwei.
|
|
||
| bn, err := c.backend.node.blockNumber(c.backend.rpcCtx) | ||
| func (c *redeemCoin) Confirmations(ctx context.Context) (int64, error) { |
There was a problem hiding this comment.
Do we ever check redeem confs? I'm drawing a blank on this for some reason
There was a problem hiding this comment.
I don't think so. I put added a TODO in the (*Backend).newRedeemCoin docs about it.
dex/networks/eth/params.go
Outdated
| vGwei := new(big.Int).Div(v, BigGweiFactor) | ||
| if vGwei.IsUint64() { | ||
| return vGwei.Uint64() | ||
| } |
There was a problem hiding this comment.
Two things I was trying to avoid with the ToGwei.
- A package level BigGweiFactor. This is mutable. Is this safe? For one a consumer could accidentally mutate it.
bigisn't the easiest package to use imo. Whenever you havea := b.Add(c, d )you might not expect b to change here, because we just want to add c and d, but b is set to those and returned as a. Also, could an asset that is not one of ours be added that maliciously changes this? I'm not sure if that is something we are worried about, but imo starting with a constant, or at least not exporting it, is safer.
As an example, add this test to the top of eth_test.go and another test will fail:
func TestMutate(t *testing.T) {
fmt.Println("**************")
fmt.Println(dexeth.BigGweiFactor)
c := big.NewInt(2)
d := big.NewInt(3)
a := dexeth.BigGweiFactor.Add(c, d)
fmt.Printf("%p %p\n", a, dexeth.BigGweiFactor)
fmt.Println(dexeth.BigGweiFactor)
}
- A
big.Intmay not fit into auint64. It could be too large or negative. It would be unexpected, but have untested consequences. Should we at least panic?
| txdata := tx.Data() | ||
| // Transactions that call contract functions must have extra data. | ||
| if len(txdata) == 0 { | ||
| return nil, errors.New("tx calling contract function has no extra data") | ||
| } | ||
|
|
There was a problem hiding this comment.
Was this check not accurate? I thought it was a good way to fail fast.
There was a problem hiding this comment.
baseCoin is more generalized. The txdata length check is now performed (via ParseInitiateData) in newSwapCoin immediately after the baseCoin is created.
| // For redemptions, the transaction should move no value. | ||
| if sct == sctRedeem && value != 0 { | ||
| return nil, fmt.Errorf("expected swapCoin value of zero for redeem but got: %d", value) | ||
| } |
There was a problem hiding this comment.
This check removed. Not needed? Any redeem tx with a value should fail to redeem because the solidity redeem function is not marked payable.
There was a problem hiding this comment.
Value is now being checked (to a tighter standard) in newSwapCoin.
There was a problem hiding this comment.
Isn't that for inits, I mean redeems should be 0 value.
| if err != nil { | ||
| return 0, fmt.Errorf("unable to fetch block number: %v", err) | ||
| } | ||
| return int64(bn - swap.InitBlockNumber.Uint64()), nil | ||
| return int64(bn - swap.BlockHeight + 1), nil |
Separate swapCoin from redeemCoin. Switch (*rpcclient).swap method to return a *dexeth.SwapState.
buck54321
force-pushed
the
erc20-coiner-refactor
branch
from
64dc3ae to
d597dc5
Compare
