solana-labs / solana

Web-Scale Blockchain for fast, secure, scalable, decentralized apps and marketplaces.
https://solanalabs.com
Apache License 2.0
13.19k stars 4.3k forks source link

RPC: Add destination owner to parsed token transfer instruction response #17565

Closed jstarry closed 3 years ago

jstarry commented 3 years ago

Problem

Wallets typically send SPL tokens between "owner" accounts but the json-rpc parsed token instruction response doesn't return the necessary information to determine the owner of the destination token account. Another request is needed to fetch the destination token account (which may no longer exist)

Proposed Solution

Add destinationAuthority to the parsed response:

{
    "parsed": {
        "info": {
            "amount": "5000000",
            "authority": "DnnP47zbPYaQXdyvm9C1j6vXboCHfcbrVSuSgFjB9Cmg",
            "destination": "Cx1xn6Fqbwsvy4LiN4oR8L5XiEjJV1LEtFArgd3zfihF",
            "source": "C7eAki6kLpRzer2jQBKCYMo2bPCWikWb9ej1G3kWQtLN",
            // proposed addition:
            "destinationAuthority": "DnnP47zbPYaQXdyvm9C1j6vXboCHfcbrVSuSgFjB9Cmg"
        },
        "type": "transfer"
    },
    "program": "spl-token",
    "programId": "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA"
}
jstarry commented 3 years ago

@CriesofCarrots how do you feel about this addition?

CriesofCarrots commented 3 years ago

@jstarry sorry for the delay! Missed this notification.

I do like that idea conceptually, but I don't see a path forward that's appreciably better than doing the query client-side. The instruction-parsing code operates strictly on a Message, without the ability to query for additional context. It is possible to pass that information in from the rpc processor, but a lot of churn; and other parsers would no longer be able to operate fully without it. Even then, the issue of the destination token account no longer existing (which you mentioned above) would still apply, since parsing via rpc may be run on an instruction at any point in ledger history, and we don't have historical account state stored.

Another way to approach this might be something similar to what TransferChecked does with the mint, where the destination authority account is actually included in the instruction. But then we'd be looking at redeploying SPL token, and no guarantee that clients would use it (see: current TransferChecked adoption).

jstarry commented 3 years ago

No worries @CriesofCarrots! Thanks for the response, it makes a lot of sense. I think you're right that we would need to make changes to the way the token program works to make headway here. I'll move the issue over to the SPL repo then for discussion

ryoqun commented 3 years ago

(@CriesofCarrots @jstarry @t-nelson: well, it seems that this information in getBlock might be attracting purely from the integration perspective. I'm thinking there could be some way to achieve similar outcome...)

ryoqun commented 3 years ago

(@CriesofCarrots @jstarry @t-nelson: well, it seems that this information in getBlock might be attracting purely from the integration perspective. I'm thinking there could be some way to achieve similar outcome...)

further, related comment here is that once transaction v2 go live, getconfirmblock might be forced to store additional info into blockstore to keep returning same set of fields, iirc.

how about piggybacking onto this?

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any activity in past 7 days after it was closed. Please open a new issue for related bugs.