orca-so / orca-sdks

Open-sourced typescript SDKs for Orca
MIT License
43 stars 10 forks source link

Added a WrappedSolAccountCreateMethod (ata) using syncNative #66

Closed wjthieme closed 5 months ago

wjthieme commented 5 months ago

This PR adds a third WrappedSolAccountCreateMethod using syncNative instruction. Benefit of this method is that if you don't have to add any sol from the beginning you'd only have 2 instructions (as opposed to 3 with keypair or seed method). This method also does not require an extra signer.

wjthieme commented 5 months ago

Good point! I think transaction size is only an issue if all 5 ATAs need to be created and wSOL is one of the tokens. This is a very unlikely scenario since we almost never have 3 reward tokens that are different from the trade tokens. Might be worth just only forcing ata method in that specific case and keeping keypair as default? Will add a couple of tests to whirlpool sdk for this if this sounds good

Alternatively we can just let this case fail loudly (like it does now in the last version of the whirlpool sdk pr) and tackle it at a future point if it ever occurs

yugure-orca commented 5 months ago

@wjthieme I know it sounds very conservative, but considering the developers who use common-sdk, I think the default behavior should not be changed.

I completely agree that the case of making 5 token accounts is quite rare. And even if there are multiple TransactionBuilders, it does not necessarily mean that they will fail. It is a matter of the difficulty of simulating two sequential transactions in the wallet app, and if the simulation is ignored, it would be possible to execute.

Based on this, it may simply be a matter of merging the two TransactionBuilders if there is no problem in merging them. This would make it a single TransactionBuilder except in rare cases, and even in rare cases, the results returned by the SDK would not be wrong. I think this change would not require any changes other than closePosition method. (This can be accomplished by changing only the whirlpools-sdk)

size check & merge builders: https://github.com/orca-so/whirlpools/blob/main/sdk/src/instructions/composites/collect-all-txn.ts#L174-L183 (latestBlockhash can be dummy data...)

wjthieme commented 5 months ago

Yes I'll change the default back to keypair. Agree with the reasoning

My follow up was regarding the edge case of 5 ATAs plus wSOL. I think there are a few options for handling just that edge case: