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

Docs: Add SPL Token exchange integration #12303

Merged
merged 8 commits into from
Sep 18, 2020

Conversation

t-nelson
Copy link
Contributor

@t-nelson t-nelson commented Sep 17, 2020

Problem

No documentation to assist exchanges in integrating SPL Token

Summary of Changes

Add some

Fixes #12281

TODO

  • Add --verbose flag to spl-token balance for also printing the mint
  • Recommend checking withdraw accounts with ☝️

I'm going to punt the TODO to solana-labs/solana-program-library#467

@t-nelson
Copy link
Contributor Author

Do we want to mention the existing JS (no docs published yet) or RPC (explicitly tagged unstable) at this point?

@mvines
Copy link
Member

mvines commented Sep 17, 2020

Do we want to mention the existing JS (no docs published yet) or RPC (explicitly tagged unstable) at this point?

Na, just another place to update when that's no longer the case IMO.

@mvines
Copy link
Member

mvines commented Sep 17, 2020

Let's take the opportunity to flesh out the "Listening for Deposits" section, for both SOL and tokens. General recommendation is that the getConfirmedBlocks and then getConfirmedBlock RPC methods be used to monitor the chain for deposits into the exchange user accounts.

For SOL, scanning the transaction meta postBalances for increases to any deposit account is sufficient.
For tokens, they'd need to check for spl-token transfer instructions into a deposit address and then invoke getTokenAccountBalance on the deposit account to read it's current balance. This nicely adds a dependency on @jstarry's work to expose CPI instructions in RPC as well, since the exchange wouldn't want to miss those deposits

@mvines
Copy link
Member

mvines commented Sep 17, 2020

There's probably a small handful of issues that are going to come out of this effort @t-nelson. Consider creating a Github project so that it's easy to track them all and we can have different folks working on some in parallel potentially.

@mvines
Copy link
Member

mvines commented Sep 17, 2020

This seems to be the best withdrawal flow:
User provides their primary wallet address. Exchange creates a new spl-token account and assigns authority of it to the user's wallet. This will require wallets like Sollet/Solflare to support merging of spl-token accounts (but that seems generally useful regardless).

Deposit flow is through a normal spl-token transfer operation

@t-nelson
Copy link
Contributor Author

New project: https://github.com/orgs/solana-labs/projects/2

I've populated it with the stuff I'm aware of so far

@t-nelson
Copy link
Contributor Author

This seems to be the best withdrawal flow:
User provides their primary wallet address. Exchange creates a new spl-token account and assigns authority of it to the user's wallet. This will require wallets like Sollet/Solflare to support merging of spl-token accounts (but that seems generally useful regardless).

Is this going to fit into existing withdraw UX flows without additional integration overhead?

@t-nelson
Copy link
Contributor Author

t-nelson commented Sep 17, 2020

For SOL, scanning the transaction meta postBalances for increases to any deposit account is sufficient.

Wasn't there discussion of extending postBalances with SPL Token balance deltas as well? More of a "wouldn't it be nice" than a "let's do it right now" comment, the suggestion sounds fine in the interim

@mvines
Copy link
Member

mvines commented Sep 17, 2020

This seems to be the best withdrawal flow:
User provides their primary wallet address. Exchange creates a new spl-token account and assigns authority of it to the user's wallet. This will require wallets like Sollet/Solflare to support merging of spl-token accounts (but that seems generally useful regardless).

Is this going to fit into existing withdraw UX flows without additional integration overhead?

I think so. Withdraw flow is pretty simple: user provides address and amount. In this case the address is for the account authority. Then rather than running a solana transfer <address> <amount> to execute the withdraw, it's some spl-token ... commands

@mvines
Copy link
Member

mvines commented Sep 17, 2020

For SOL, scanning the transaction meta postBalances for increases to any deposit account is sufficient.

Wasn't there discussion of extending postBalances with SPL Token balance deltas as well? More of a "wouldn't it be nice" than a "let's do it right now" comment, the suggestion sounds fine in the interim

Ah that's probably a better solution. I think we can scope out both and then decide if we want to skip the interim option entirely

@t-nelson
Copy link
Contributor Author

I think so. Withdraw flow is pretty simple: user provides address and amount. In this case the address is for the account authority. Then rather than running a solana transfer

to execute the withdraw, it's some spl-token ... commands

Yeah, I guess my concern is that there will be significant education required to communicate the indirection here. I could see a user providing an actual SPL Token account for w/d, which was created with a throwaway keypair for instance, which would be bad.

Ah that's probably a better solution. I think we can scope out both and then decide if we want to skip the interim option entirely

I'll add an issue!

@t-nelson t-nelson force-pushed the docs-spl-tok-exc-int branch from 27854b6 to 4f43db6 Compare September 18, 2020 03:10
@t-nelson t-nelson marked this pull request as ready for review September 18, 2020 03:11
@t-nelson t-nelson force-pushed the docs-spl-tok-exc-int branch from 4f43db6 to 3246156 Compare September 18, 2020 03:17
@t-nelson t-nelson force-pushed the docs-spl-tok-exc-int branch from 3246156 to dbc5ba5 Compare September 18, 2020 03:18
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
docs/src/integrations/exchange.md Show resolved Hide resolved
docs/src/integrations/exchange.md Outdated Show resolved Hide resolved
CriesofCarrots
CriesofCarrots previously approved these changes Sep 18, 2020
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

One typo, then lgtm!

@mergify mergify bot dismissed CriesofCarrots’s stale review September 18, 2020 19:50

Pull request has been modified.

@t-nelson
Copy link
Contributor Author

@mvines does this cover our bases now?

mvines
mvines previously approved these changes Sep 18, 2020
docs/src/integrations/exchange.md Show resolved Hide resolved
@mvines
Copy link
Member

mvines commented Sep 18, 2020

@mvines does this cover our bases now?

Yep! I think this is a great start. There's a number of improvements of course but that's all incremental work

@t-nelson t-nelson added the automerge Merge this Pull Request automatically once CI passes label Sep 18, 2020
@mergify mergify bot dismissed mvines’s stale review September 18, 2020 20:54

Pull request has been modified.

@t-nelson t-nelson removed the automerge Merge this Pull Request automatically once CI passes label Sep 18, 2020
@t-nelson t-nelson force-pushed the docs-spl-tok-exc-int branch from 3c757bc to aca7f6b Compare September 18, 2020 21:12
@t-nelson t-nelson added automerge Merge this Pull Request automatically once CI passes v1.3 labels Sep 18, 2020
@mergify mergify bot merged commit a695561 into solana-labs:master Sep 18, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 18, 2020

automerge label removed due to a CI failure

@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Sep 18, 2020
@t-nelson t-nelson deleted the docs-spl-tok-exc-int branch September 18, 2020 21:37
@CriesofCarrots CriesofCarrots added v1.3 and removed v1.3 labels Sep 18, 2020
t-nelson added a commit to t-nelson/solana that referenced this pull request Sep 19, 2020
mergify bot pushed a commit that referenced this pull request Sep 19, 2020
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.

Add an section for SPL Token integration to the exchange integration guide
3 participants