aeternity / dex-contracts-v2 Goto Github PK

This issue is opened as a general point to add new tests scenarios/proposals for additional tests that are not there (test/*.test.js)

please feel free to add any comments with things that you find appropriate on this subject

There was an error on Mdw where a contract was not identified as an AEX9.

Adding the aex9_extensions entrypoint to the minimal interface is important to prevent similar cases once it's a per-requisite for a contract to be considered per spec an AEX9 token.

add contract interfaces:

• IAEX9Minimal
• IAedexV2Callee
• IAedexV2Token
• IAedexV2Factory
• IAedexV2Pair

• replace old style (exe) of calling entrypoints with new one from the fixtures
• remove old style from fixtures

add a contract which wraps main pair actions (swap,mint etc) and implements IAedexV2Callee. this contract will be used only in tests

we should review and validate all the entrypoint inputs that should be positive

see aeternity/dex-ui#361 (comment)

used compiler version: v6.1.0

based on #10 (comment)

add more description also

Based on the discussion:

@thepiwo thepiwo 11 days ago
the contract should not be payable, only the function to accept AE
  
Author
@bogdan-manole bogdan-manole 6 days ago
at remove_liquidity_ae the Router is paying back AE through Chain.spend, if I remove payable from the contract declaration i will get this error:
Error: Invocation failed: "Error in spend: account_is_not_payable"
  
@thepiwo thepiwo 4 days ago
I feel this is bad practice, but I can't think of a way to do this differently (maybe WAE can spend directly to receiver?)

add contract for AedexV2Factory

frac cant be used? https://github.com/aeternity/aesophia/blob/master/docs/sophia_stdlib.md#func

Originally posted by @thepiwo in #10 (comment)

original discussion:

@thepiwo thepiwo on 9 Nov
frac cant be used? https://github.com/aeternity/aesophia/blob/master/docs/sophia_stdlib.md#func

@thepiwo thepiwo 25 days ago
still open question

Author
@bogdan-manole bogdan-manole 10 days ago
I've tried to make the translation to it, and it took me a while with not so much success for the tests and other matter I don't recall right now.
I did not have enough time to do it though.
I think we should put it as a separate issue if we want this. And also discuss how we want to proceed further:

for example in which format we want to return the value: two separate numbers (numerator and denominator) or we encode it somehow (for example as UQNxM :) ) ?

@thepiwo thepiwo 5 days ago
ok, lets keep it like this

some extra review and possible any additional extra AEX9 entrypoints that should be added to WAE contract

• global contract deployments
• contract deployments for tests (fixture)
• js utility functions

Just wanted to write this down as we discussed this in Sofia:
What are the essential operations here to look into? I'm not a DEXuser so please come forward with suggestions. The information should be available in the tests I think, could you log them and share them here @bogdan-manole?

This is important in order to compare the performance of our DEX with the ones on Ethereum.

add contract main libraries:
�Math
�SafeMath
�UQ112x112

see https://raw.githubusercontent.com/aeternity/aeternity/master/LICENSE

this is basically an AEX-9 implementation right? Why does it deviate so much from the reference implementation

Originally posted by @thepiwo in #10 (comment)

original comments:

epiwo 5 days ago
this is basically an AEX-9 implementation right? Why does it deviate so much from the reference implementation

@thepiwo thepiwo 5 days ago
if this is in the test folder I assume its not needed in prod, but it will be right? (if just for tests maybe move to /test)

Author
@bogdan-manole bogdan-manole 3 days ago
i've just brought the uniswap implementation , i will bring ours.
about the test , the same reason, but I will move it to the root level

• The feature that the pair is created if it doesn't exist (when adding
  liquidity) is somewhat hidden - more documentation will help.

aepp-sdk => v10.0.0

• currently beta version is used

aeproject => v4.0.0 (or v4.0.0.alpha)

• https://github.com/aeternity/dex-contracts-v2/blob/master/.github/workflows/main.yml#L19
• npm i @aeternity/[email protected]

As we are working on deprecating the compiler, please generate and export the ACIs.

a decision should be made regarding hardcoded name and symbol of Pair's implementation

DESCRIPTION:
Changing allowance in Pair with a bigger value than previous one, will give "AedexV2Pair: INSUFFICIENT_ALLOWANCE" error

EXPECTING:
to succeed executing transaction

Add the following disclaimer to the contracts:

Under no circumstances, whether in tort (including negligence), contract, or otherwise, unless required by applicable law, shall Aeternity Anstalt be liable for damages, including any direct, indirect, special, incidental, or consequential damages of any nature arising out of the deployment or use of this smart contract, notwithstanding that Aeternity Anstalt may have been advised of the possibility of such damages.

as discussed this doesn't exist right now. would be nice to have and be documented of course

The type of pairs in the state is a nested map, it would be better (more
efficient and less error-prone) to use the type map({IAEX9Minimal, IAEX9Minimal}, IAedexV2Pair)

• Comment on the debug functionality that it can only be on if the Factory is configured with it on.

• Duplicated checks? For example, will L73 require(amount_a_optimal =< amount_a_desired, "AedexV2Router: OPTIMAL_GREATER_THEN_DESIRED") ever
  trigger?

add contract for AedexV2Pair

• main action implementation (swap, mint, burn etc)
• aex9 implementation for liquidity token

remove the parameter from it and replace it with Contract.address

-Naming of (helper) functions, add_liquidity' for example does not add any liquidity but makes a computation about liquidity.

if so we need to check this in the payable function, is there really AE native coins expected to be received? Only via the proxy, then check it? Or should it be wrappedAE, then payable can't be used, in default AEX-9 there is no safe transfer callback, but if we need one specifically for wrappedAE this can be done (keep in mind reentry into an already called contract with remote calls is not possible WAE -> Router -> WAE for receive check won't be possible)

Originally posted by @thepiwo in #10 (comment)

!!! they come back sorted, so we can't assume the order here

Originally posted by @thepiwo in #10 (comment)

@thepiwo thepiwo 11 days ago
!!! they come back sorted, so we can't assume the order here

@thepiwo thepiwo 11 days ago
maybe we should return records that indicate we operate on SORTED or UNSORTED token/amount

Author
@bogdan-manole bogdan-manole 6 days ago •
edited
they don't come back sorted. In the Router, all the entrypoints (as remove_liquidity is) will return the values in accordance/order with what the user specify as A and B in the parameters. So, if you have Router.entrypointX(tokenA,tokenB,amountA_min,amountB_min) => (amountA * amountB), the order of the returned tuple will remain as you specified in parameters. So, remove_liquidity(token,wae) will return (amount_token*amount_wae).

We should not specify if it is sorted or unsorted since always they come back sorted by the parameters order.

One might wonder the following questions:

why Pair is not doing the same sorting/unsorting with the token0 and token1 ?, at least three reasons:

Pair does not have any of the tokenA,tokenB in the parameters of it's entrypoints (for example burn,mint,etc) to be able to understand what an caller/user might refer to tokenA/tokenB
Pair does not have to deal with the sorting/unsorting since upper level contracts will do it for it, if it is the case.
From the point of view of the Pair there is just his state.token0 and state.token1, and the fact that token0.address < token1.address, is just a convention of the Factory not of the Pair constraints. In other words the Pair doesn't care if there will be other Pair with same tokens in reverse order or even the same as it's own, or even if token0.address > token1.address. It is Factory's business to ensure any constraints of pair/tokens uniqueness or order.
why the router is doing this?

the Router has to find the pair for the user, so every time he has the tokenA and tokenB is normal to match the values given by the pair to those provided by the user
the Router in the case of the multiple swaps might have [tokenA,tokenB,tokenC] in the form of a path of tokens and it should return the same order [amountA,amountB,amountC]

@thepiwo thepiwo 4 days ago
remove_liquidity is sorting token0, _, then amount_a will be the amount of previously sorted token0 in the if/else so I feel it is sorted and we can't know the order of (amount_token, amount_ae). Or do I missunderstand that somehow?

agree

all good explainations, agree

Author
@bogdan-manole bogdan-manole 1 hour ago
The amount_a will be the amount of token0 only if token_a == token_0, otherwise will be the amount of token1. Keeping this in mind we know that remove_liquidity will always return as first value in the tuple the value representing the first token provided in the parameter for remove_liquidity => remove_liquidity(tokenA,tokenB) will return (amountA,amountB). In our case tokenA == token and tokenB == state.wae_aex9. In other words the remove_liquidity_ae is just a special case when we don't have parameters for both tokenA and tokenB but just for the tokenA, tokenB being used as state.wae

we should provide a documentation to provide more insights. ideally we serve this documentation via MkDocs like in other repositories

The state update syntax allows for shorter (and more efficient) code in
deposit et.al. For example:
put(state{balance_of @ b = b { [ Call.caller = 0] @ value = value + Call.value} })
is better written as
put(state{balance_of[Call.caller = 0] @ value = value + Call.value})