Make pair_for method internal call #20
Conversation
| /// Original Uniswap Library pairFor function calculate pair contract address without making cross contract calls. | ||
| /// Please refer https://github.com/Uniswap/v2-periphery/blob/master/contracts/libraries/UniswapV2Library.sol#L18 | ||
| /// | ||
| /// In this contract, use cross contract call to get pair contract address. | ||
| default fn _pair_for( |
There was a problem hiding this comment.
I would suggest to still leave a doc comment that this is the equivalent of CREATE2 function in solidity + link to substrate where the accountId of the deployed contract is calculated from these args.
There was a problem hiding this comment.
Agree, will add it.
| @@ -6,4 +7,6 @@ pub const STORAGE_KEY: u32 = openbrush::storage_unique_key!(Data); | |||
| #[openbrush::upgradeable_storage(STORAGE_KEY)] | |||
| pub struct Data { | |||
| pub factory: AccountId, | |||
| // TODO: remove pair code hash as like https://github.com/Uniswap/v2-periphery/blob/master/contracts/libraries/UniswapV2Library.sol#L18 | |||
| pub pair_code_hash: Hash, | |||
There was a problem hiding this comment.
I think you will need to keep this in storage.
But add a setter with onlyOwner to change it in case of a contract upgrade.
There was a problem hiding this comment.
Got it. For now, let's not add a setter since our code base doesn't support upgrade currently.
There was a problem hiding this comment.
I think you can remove the comment line
| let pair = | ||
| FactoryRef::get_pair(&factory, token_0, token_1).ok_or(RouterError::PairNotFound)?; | ||
| Ok(pair) | ||
| // TODO: remove pair code hash after deployment |
There was a problem hiding this comment.
Can you add at least one or two tests (in Rust)?
There was a problem hiding this comment.
It is quite hard to test it in rust for now (with ink 4.0 it will be possible, see use-ink/ink#1234). Ideally, we should verify the computed address is equal to queried via cross contract call; but currently we cannot do cross-contract call in rust side.
If I need to add a test, then it will be a integration test in typescript.
There was a problem hiding this comment.
I think passing existing tests is sufficient to exhibit it works fine.
There was a problem hiding this comment.
I meant a hardcoded test with Alice as deployer & pari_code hash & arbitrary salt - > and checks if is equal what you get on-chain.
But seems like the code on substrate get refactored so it can be impl as is in the contracts so there is not much risk here. So agree it can be done in TS.
However, would you agree that this generate_address should be par of ink_env crate directly? (PR idea)
There was a problem hiding this comment.
Yep, I felt also that generate_address should be available from inv_env.
Resolves #19
A good showcase how to calculate contract address even before corresponding contract gets deployed, like create2 of EVM.