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

feat: contracts #4

Open
wants to merge 29 commits into
base: contractless
Choose a base branch
from
Open

feat: contracts #4

wants to merge 29 commits into from

Conversation

0xtekgrinder
Copy link
Owner

No description provided.

Copy link

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

I left a few comments, but I didn't dive deep into the code. It would be good to get Onbloc's input, since they're more active on this type of code.

contracts/token_register.gno Outdated Show resolved Hide resolved
contracts/token_register.gno Outdated Show resolved Hide resolved
contracts/utils.gno Outdated Show resolved Hide resolved
contracts/vault.gno Outdated Show resolved Hide resolved
contracts/utils.gno Outdated Show resolved Hide resolved
@@ -0,0 +1,37 @@
package gnoswap_optimizer

func GetVault(tokenId uint64) Vault {
Copy link

Choose a reason for hiding this comment

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

Most of getters in gnoswap is meant be call from off-chain (abci_query or adena or gnokey).

If GetVault() is same purpose, you should return primitive type.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I thought this function would be more used other realms and the other getters more for rpc usage.

I just didn't split into two files as I don't have a lot of getters

contracts/gno.mod Outdated Show resolved Hide resolved
contracts/token_register.gno Outdated Show resolved Hide resolved
contracts/gno.mod Outdated Show resolved Hide resolved
contracts/token_register.gno Outdated Show resolved Hide resolved
contracts/utils.gno Outdated Show resolved Hide resolved
contracts/utils.gno Outdated Show resolved Hide resolved
contracts/vault.gno Outdated Show resolved Hide resolved
}

liquidityRatio := computeLiquidityRatio(tokenId, parsedAmount)
_, liquidity, _, _, amount0, amount1, _ := position.DecreaseLiquidity(tokenId, liquidityRatio, uint256.NewUint(0).ToString(), uint256.NewUint(0).ToString(), time.Now().Unix()+1, false)
Copy link

Choose a reason for hiding this comment

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

time.Now().Unix()+1, too short don't you think ??

Copy link
Owner Author

Choose a reason for hiding this comment

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

It isn't a order type AMM so the swap will be executed in the same transaction. So no need to add a higher deadline as it won't matter anyway

contracts/vault.gno Outdated Show resolved Hide resolved
Comment on lines +130 to +141
func NewVault(
token0 string,
token1 string,
fee uint32,
tickLower int32,
tickUpper int32,
amount0 string, // *uint256.Uint
amount1 string, // *uint256.Uint
amount0Min string, // *uint256.Uint
amount1Min string, // *uint256.Uint
deadline int64,
) {
Copy link

Choose a reason for hiding this comment

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

It seems need to validate that token0 and token1 are valid and distinct token addresses

Copy link
Owner Author

Choose a reason for hiding this comment

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

The check is already being done in the Mint function of the position realm so will eb a double check

contracts/vault.gno Show resolved Hide resolved
contracts/vault.gno Outdated Show resolved Hide resolved
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.

4 participants