token-swap: Add instructions to deposit / withdraw one token by joncinque · Pull Request #937 · solana-labs/solana-program-library

joncinque · 2020-12-09T21:46:36Z

In order to properly support the new curves, we need the ability to deposit or withdraw just one side of the token swap. This uses the interface added in #934.

Since the math part is covered in other PRs, this focuses on the interface into processor.rs, and the tests closely mirror the logic to test the normal withdraw and deposit instructions.

joncinque

Most of the tests in processor.rs mimic the checks from withdraw and deposit, so there may be some other cases to check

joncinque · 2020-12-10T19:22:22Z

    ) -> Option<u128> {
-        // Get the trading fee incurred if the owner fee is swapped for the other side
-        let trade_fee = fees.trading_fee(source_amount)?;
+        // Get the trading fee incurred if *half* the source amount is swapped


This is also done in #936

joncinque · 2020-12-10T19:22:33Z

 }

+/// Deposit one exact in instruction data
+#[cfg_attr(feature = "fuzz", derive(Arbitrary))]


Fuzzing will come later

joncinque · 2020-12-10T19:23:12Z

    }

+    #[allow(clippy::too_many_arguments)]
+    fn check_accounts(


This felt like a nice refactor :-)

joncinque · 2020-12-10T19:24:25Z

+            ),
+        };
+
+        let burn_pool_token_amount = token_swap


To do exact amount out, we have to calculate the amount and then tack the fee on afterward

joncinque · 2020-12-10T19:26:08Z

CI passes locally

CriesofCarrots

Based on my understanding of the math, this looks functionally good. Wouldn't hurt to get another pair of eyes on it, though.
I think the names for the new instructions could perhaps be more clear/precise. Wdyt?

CriesofCarrots · 2020-12-11T07:02:11Z

    ///   9. '[]` Token program id
    Withdraw(Withdraw),
+
+    ///   Deposit some tokens into the pool.  The output is a "pool" token


Based on this comment, I don't see any difference between Deposit and DepositOneExactIn. Can you make it more descriptive?

That's a good point, I didn't want to change the name of the previous instructions, but it definitely makes things clearer. We could change Withdraw / Deposit to WithdrawAll / DepositAll -- what do you think?

CriesofCarrots · 2020-12-11T07:08:20Z

+    ///   5. `[writable]` Pool MINT account, $authority is the owner.
+    ///   6. `[writable]` Pool Account to deposit the generated tokens, user is the owner.
+    ///   7. '[]` Token program id
+    DepositOneExactIn(DepositOneExactIn),


Also, I'm not crazy about DepositOneExactIn. "One" makes me think I'm depositing amount one, I'm not sure what is "exact", and "in" seems redundant. ...Is there precedent for this nomenclature elsewhere in swap-land? If not... what about DepositAndSwap?

I see what you mean... I wanted to make it clear that you're only depositing one token type instead of both, and that you're specifying exactly how much is going in, as opposed to the old deposit, which specifies the exact amount of pool tokens that you'll get out. Since it's not actually performing a swap, DepositAndSwap seems misleading. What about DepositOneSideExactAmountIn to be totally clear?

Do you expect to also support depositing one side but specifying the input in pool tokens? If not, I don't think ExactAmountIn is needed. That can just be defined in the interface.
Are we still looking toward pools of >2 tokens?
How about DepositSingleTokenType

It would be useful for people to be able to specify which side they want to fix when depositing or withdrawing, and it's something used in Balancer, so providing that info in this instruction would be best. DepositSingleTokenType definitely clarifies the One part -- what about DepositSingleTokenTypeExactAmountIn? For reference, I was looking at Balancer's terminology at https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol -- but I found functions like joinswapExternAmountIn opaque, so I'm very motivated to make this clear for us!

So hard to find something both precise and concise! ;)
DepositSingleTokenTypeExactAmountIn is at least very clear... wfm

CriesofCarrots · 2020-12-11T07:11:31Z

        Ok(())
    }

+    /// Processes DepositOneExactIn


Suggested change

/// Processes DepositOneExactIn

/// Processes [DepositOneExactIn](enum.Instruction.html)

I've been wondering -- what does this do?

These are just markdown links for when the docs are published to docs.rs, eg: https://docs.rs/spl-token/3.0.0/spl_token/processor/struct.Processor.html
But it looks like we're not composing those quite right in spl-token; they need to go to ../instruction/enum.TokenInstruction.html 😳. And here, they also need to be renamed to SwapInstruction. Feel free to ignore for the time being, but let's get these updated before we publish a v1 to crates.io

Could we use the simpler markdown as in https://doc.rust-lang.org/rustdoc/linking-to-items-by-name.html? I'm surprised this didn't get caught by the lints. Definitely something else to look into

Oh cool, it looks like it; that's way better. I haven't dug into rustdoc as I should

I only looked into it just now 😆

CriesofCarrots · 2020-12-11T07:11:47Z

+        Ok(())
+    }
+
+    /// Processes an [Withdraw](enum.Instruction.html).


Suggested change

/// Processes an [Withdraw](enum.Instruction.html).

/// Processes an [WithdrawOneExactOut](enum.Instruction.html).

CriesofCarrots · 2020-12-11T07:12:12Z

+    ///   6. `[writable]` token_(A|B) User Account to credit
+    ///   7. `[writable]` Fee account, to receive withdrawal fees
+    ///   8. '[]` Token program id
+    WithdrawOneExactOut(WithdrawOneExactOut),


Ditto with this instruction name

WithdrawOneSideExactAmountOut for this?

joncinque · 2020-12-15T19:54:07Z

This should be good to go with the renames now!

CriesofCarrots

Much better. Thanks for the comment expansion as well!

joncinque force-pushed the ts-insts branch from 848605a to 68cbf67 Compare December 10, 2020 19:16

joncinque changed the title ~~WIP token-swap: Add instructions to deposit / withdraw one token~~ token-swap: Add instructions to deposit / withdraw one token Dec 10, 2020

joncinque marked this pull request as ready for review December 10, 2020 19:17

joncinque commented Dec 10, 2020

View reviewed changes

joncinque requested a review from CriesofCarrots December 10, 2020 19:26

joncinque mentioned this pull request Dec 11, 2020

token-swap: Add constant price and offset curves #922

Closed

joncinque force-pushed the ts-insts branch from cac2913 to 3752d8b Compare December 11, 2020 13:15

joncinque added 6 commits December 14, 2020 18:10

Add instructions to deposit / withdraw one token

bf82866

Run cargo fmt

04d12a8

Fix clippy issues

0e957b1

Add JS interface

532c51b

Add tests for new instructions

b4af3b4

Run prettier

996eac5

joncinque force-pushed the ts-insts branch from 3752d8b to 996eac5 Compare December 14, 2020 17:13

CriesofCarrots reviewed Dec 15, 2020

View reviewed changes

joncinque added 3 commits December 15, 2020 20:36

Rename deposit and withdraw

dc86b5f

Rename withdraw -> withdraw all in program

8961133

Rename single token instructions

a1c12b3

CriesofCarrots approved these changes Dec 15, 2020

View reviewed changes

joncinque merged commit 7190672 into solana-labs:master Dec 16, 2020

joncinque deleted the ts-insts branch December 21, 2020 16:29

	/// Processes DepositOneExactIn
	/// Processes [DepositOneExactIn](enum.Instruction.html)

	/// Processes an [Withdraw](enum.Instruction.html).
	/// Processes an [WithdrawOneExactOut](enum.Instruction.html).

Uh oh!

Conversation

joncinque commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joncinque commented Dec 10, 2020

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joncinque commented Dec 15, 2020

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joncinque commented Dec 9, 2020 •

edited

Loading