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

add swap function, queries #3

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

add swap function, queries #3

wants to merge 13 commits into from

Conversation

wc117
Copy link
Contributor

@wc117 wc117 commented Nov 1, 2022

No description provided.

@measure-fi measure-fi self-requested a review November 1, 2022 13:12
new_admin,
new_event_tracker,
} => {
STATE.update(deps.storage, |mut state| -> StdResult<_> {
Copy link
Contributor

Choose a reason for hiding this comment

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

State/STATE &c may be better named Config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

State/STATE includes configuration and pool_count which is not a part of config.

Comment on lines +131 to +133
if let Some(new_admin) = new_admin {
state.admin = new_admin;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One question I had, what is the point of being able to transfer admin account vs having a list of admins (or similar)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if we don't have to implement multiple admins, a single admin will be better for security.

src/msg.rs Outdated
Comment on lines 158 to 159
/// Pool id.
pool_id: Uint256,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's hard, but this cannot be the documentation for pool id.

  1. You can make a newtype struct PoolId(Uint256) and document that (what is a pool id?), then this documentation becomes a little forgivable.
  2. OR You can describe here what a pool id is and what it's used for. Are liquidity elements identified by their pool id (one per pool?), the queue id? Both? &c.
  3. Or you can do some combo. Newtypes are a little rust specific but they would go a long way towards clarifying this code.

(this applies everywhere there is an

/// X.
x

style item)

@wc117 wc117 requested a review from measure-fi November 1, 2022 20:53
src/contract.rs Outdated
@@ -122,9 +171,10 @@ fn create_pool(
token1: String,
chain0_init_depositor: String,
chain1_init_depositor: String,
fee: u16,
) -> Result<Response<PalomaMsg>, ContractError> {
assert!(chain0_id < chain1_id);
Copy link

@fieldtheory123 fieldtheory123 Nov 2, 2022

Choose a reason for hiding this comment

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

If we don't assert this inequality, is it going to create problems somewhere? This design requires the two chains to be different, which is not necessary.

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