Hi there! We were reviewing Helios on a a Solidity Friday (link to shameless self-plug here), and had an observation we'd like to share. First I just wanted to say that this is a really cool idea, and we really enjoyed reading through it.

As of now, when ETH is sent while creating a pair, ETH becomes the token0 of the pair, and is represented by the zero address. This assignment is done after token0 and token1 are sorted, ensuring that token0 is the address with the higher number (source). This means that in the current structure, the following considerations must be made when creating pairs with ETH:

• two addresses must be passed into the function (source)
• the lower of these addresses will be the token paired with ETH
• the higher will be replaced by the zero address, which is used when ETH is in a pair

This would make the "best" address to pass in with the token address to pair with ETH 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF, since it will be higher than any other address passed in, and will then be replaced with the zero address for ETH. I'd like to recommend changing this. For one, it makes ETH pairs behave differently than all other pairs - in all other pairs the higher address is token0, whereas since ETH is represented by the zero address, in ETH pairs it will always be lower address first. In addition, 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF is just a bit of a pain to work with.

One possibility would be replacing token1 with the zero address to represent ETH instead of token0. This would make it possible to pass in the zero address as one of the token addresses, which is easy to use both in Solidity (address(0)) and in helper libraries in other languages (eg ethers.constants.AddressZero in ethers.js). It will always be sorted as token1 too, and ETH pairs will behave just like any other pair in terms of ordering by the higher address first.

This would mean taking this and replacing it with:

// if ETH attached, overwrite token0 and token0amount
if (msg.value != 0) {
    token1 = address(0);
    token1amount = uint112(msg.value);
    token0._safeTransferFrom(msg.sender, address(this), token0amount);
} else {
    token0._safeTransferFrom(msg.sender, address(this), token0amount);
    token1._safeTransferFrom(msg.sender, address(this), token1amount);
}

I wanted to open this as an issue instead of a PR just because I figured you may have additional reasons why you did things this way, but would be happy to make a PR myself if you'd like.

Cheers!

we should track how Helios performs against UniV2 and other contemporary exchanges

I just noticed that there was a discussion about transferring repository into foundry from hardhat.

At the same time, I've also translated template from vanilla JS to typescript support (helps in scripting). Here is my fork, I re-wrote all of the test and created few quick helper functions https://github.com/shortdoom/Helios

I do plan to tackle some Issues so it would be nice to decide for a template. Vanilla JS hardhat definitely makes least sense.

I also use foundry and it beats hardhat in speed and ease of testing but at the same time has pretty poor support for scripting and more advanced ops involving local network. It's possible that addon of hardhat/vanilla JS inside of foundry template would be necessary at some point anyways.

we should allow users to make deposits in order to save gas on frequent swapping and also to take advantage of rewards

we should add a univ3-style concentrated swapper extension. Examples include primitive and sushi trident

we should allow anyone to add reward tokens for LP-ng to a given Pool ID

Relevant: #10

Both dynamic price pools and router would be most likely interconnected from protocol design perspective, therefore I put them together in one issue.

Going by what I mentioned in #10 about basket pools/multiple token pools, I think it's more interesting to choose a different direction for design of this protocol:

1. by dynamic price pools I understand what Balancer defines as WeightedPools, e.g pools of proportion different than usual 50/50 for assetA & assetB. Another example of dynamic price change pools would be liquidity mining pools. Both of those could be implemented in Helios, with Balancer WeightedMath used as base. Relevant mechanics is described here
2. router contract to allow swaps between any token pools A <> (B) <> C. an interesting solution would be to maybe use EIP-4626 for this, but I haven't yet thought this fully through if it makes sense. of course, this can be solution outsourced from what is available in protocols like uni/bal, but making it based on 4626 could potentially mean better integration with different DeFi apps in future.

linking this discussion on use of msg.value in payable functions and multicall

we should make sure pattern for XYK extension is reasonably sound as this will be first demo deployment

how can we elegantly weave in LP fee concept?

Pretty sure there is an issue with the _mint revert conditions, and the safeTransfer and batch methods probably have the same problem. I converted the repo to foundry and had an issue minting to an EOA (which didn't seem to be an issue when I initially ran the test as a hardhat repo). Converting the if/revert condition to the require statement from solmate made the method work as expected.

currently Helios is designed to store a "pair" of assets for a given LP 1155 Id.

in the wild, there are designed for weighted "baskets" of assets, such as the Balancer LP model for index-style pools.

Two questions:
(a) how common or useful are baskets for AMM users in practice? That is, do most users nonetheless create pairs in Balancer.
(b) how difficult to implement basket array in Helios model?

Hello,

In Helios line 103, token0 is assigned to the BIGGER one. But in line 106, token0 is assigned to the SMALLER one. Token amounts are also assigned accordingly.

Also, having two similar comments on the lines 96 and 98 made me think that there was a problem in this part, and the old and new version were mixed. If this is true, and this is the latest stable version, I recommend removing between lines 97-105.