Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Felix/sdk #19

Merged
merged 48 commits into from
Nov 20, 2023
Merged

Felix/sdk #19

merged 48 commits into from
Nov 20, 2023

Conversation

FelixGibson
Copy link
Collaborator

No description provided.

#[generate_trait]
impl WrapFactoryInterfaceImpl of WrapFactoryInterface {
fn create_wrap_address(ref self: ContractState, erc1155_address: ContractAddress, token_id: u256, name: felt252, symbol: felt252) {
let wrap_address = self.map.read((erc1155_address, token_id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should move this to helper fn

import JSBI from 'jsbi';


export abstract class TickMath {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be a class but rather just be helper functions

const sortedTokens: Contract[] = [Wrap.ERC20Contract, Wrap.WERC20Contract].sort((a, b) => a.address.localeCompare(b.address));


const approveForAll: Call = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Calls should be outside of this function so they can be composed

const werc20AmountIn = BigInt(erc1155AmountIn.toString()) * BigInt(10 ** 18);
Decimal.set({ precision: 78 });
let sqrtRatioLimitX128 = (Wrap.ERC20Contract.address < Wrap.WERC20Contract.address) ? new Decimal(currentPrice / 300).sqrt().mul(new Decimal(2).pow(128)).toFixed(0) : new Decimal(currentPrice * 300).sqrt().mul(new Decimal(2).pow(128)).toFixed(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these is hard to understand - could it be decoupled?

// TODO check length
const sortedTokens: Contract[] = [Wrap.ERC20Contract, Wrap.WERC20Contract].sort((a, b) => a.address.localeCompare(b.address));
if (slippage < 0 || slippage > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be helper function

let sqrtRatioLimitX128 = (Wrap.ERC20Contract.address < Wrap.WERC20Contract.address) ? new Decimal(currentPrice / 300).sqrt().mul(new Decimal(2).pow(128)).toFixed(0) : new Decimal(currentPrice * 300).sqrt().mul(new Decimal(2).pow(128)).toFixed(0);

const approveForAll: Call = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calls should be outside of this function for composibility

const tokenIdCairo = cairo.uint256(tokenId);
const balance = await Wrap.ERC1155Contract.balance_of(address, tokenIdCairo);
return balance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return balance
return await Wrap.ERC1155Contract.balance_of(address, tokenIdCairo);

Copy link
Contributor

@ponderingdemocritus ponderingdemocritus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start

Have added some comments around the place

One major thing we should change is passing the Starknet 'Account' into the Wrap Class and having execute functions within the Class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't needed if bun exists

const [positionId, setPositionId] = useState(0);
const [liquidity, setLiquidity] = useState(0);

const erc1155_address = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can just be const

@ponderingdemocritus ponderingdemocritus merged commit 9d6c414 into main Nov 20, 2023
2 checks passed
@ponderingdemocritus ponderingdemocritus deleted the felix/sdk branch November 20, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants