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

[Solana]: Add utils to get/set compute unit price/limit #4085

Merged
merged 7 commits into from
Oct 31, 2024

Conversation

satoshiotomakan
Copy link
Collaborator

@satoshiotomakan satoshiotomakan commented Oct 29, 2024

Description

We want to be able to update ComputeBudgetInstruction::SetComputeUnitPrice and ComputeBudgetInstruction::SetComputeUnitLimit instructions for every transaction goes through Trust Wallet including Swap transactions.

Context: Rango (later Amber), Jupiter and most of the providers return a serialized transaction to sign and broadcast.

Solution: We should be able to deserialize the given raw transaction, add or update ComputeBudgetInstruction::SetComputeUnitPrice and ComputeBudgetInstruction::SetComputeUnitLimit instructions, compile the transaction again, and sign it.

How to test

Run Rust, C++, iOS, Android tests

Types of changes

  • SolanaTransaction.getComputeUnitLimit(encodedTransaction: String) -> String?, where:
    • encodedTransaction - base64 encoded transaction
    • return - optional Unit Limit as a decimal string
  • SolanaTransaction.setComputeUnitLimit(encodedTransaction: String, limit: String) -> String, where:
    • encodedTransaction - base64 encoded transaction
    • limit - decimal string
    • return - base64 encoded updated transaction
  • SolanaTransaction.getComputeUnitPrice(encodedTransaction: String) -> String?, where
    • encodedTransaction - base64 encoded transaction
    • return - optional Unit Price as a decimal string
  • SolanaTransaction.setComputeUnitPrice(encodedTransaction: String, price: String) -> String, where:
    • encodedTransaction - base64 encoded transaction
    • price - decimal string
    • return - base64 encoded updated transaction

Checklist

  • Create pull request as draft initially, unless its complete.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • If there is a related Issue, mention it in the description.

If you're adding a new blockchain

  • I have read the guidelines for adding a new blockchain.

@satoshiotomakan satoshiotomakan self-assigned this Oct 31, 2024
@satoshiotomakan satoshiotomakan marked this pull request as ready for review October 31, 2024 09:59
@satoshiotomakan
Copy link
Collaborator Author

Hi @10gic, could you please review the PR as it's related to #4082

Copy link

github-actions bot commented Oct 31, 2024

Binary size comparison

➡️ aarch64-apple-ios: 12.20 MB

➡️ aarch64-apple-ios-sim: 12.21 MB

➡️ aarch64-linux-android: 15.67 MB

➡️ armv7-linux-androideabi: 13.37 MB

➡️ wasm32-unknown-emscripten: 11.11 MB

@satoshiotomakan satoshiotomakan merged commit f6d8a10 into master Oct 31, 2024
13 checks passed
@satoshiotomakan satoshiotomakan deleted the s/solana-fee-improvement branch October 31, 2024 14:30
return tx.to_base64().tw_err(|_| SigningErrorType::Error_internal);
}

// `ComputeBudgetInstruction::SetComputeUnitLimit` should be at the beginning of the instructions list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @satoshiotomakan, I was curious why we should place the SetComputeUnitLimit at the beginning of the instructions list. Could you provide more detailed information?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @10gic, I won't try to explain better than this article :)
https://www.helius.dev/blog/priority-fees-understanding-solanas-transaction-fee-mechanics
Jump into the Best Practices section

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @satoshiotomakan, thanks for sharing this link. I suspect that this is outdated information. I found a transaction with 9 instructions (https://explorer.solana.com/tx/2Sca3UiTEaqQ7B9xPsL36gW8zRuBMRK5vDeUGG2Twe8zYfrVz1pwvAzbKHgRERsnDbVKQ6CUGCGJF1BiSQSafCCX), and the SetComputeUnitLimit is the last instruction.

In this transaction, the 7th Instruction consumed more than the default limit (200000):

 Program consumed: 243455 of 354569 compute units

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good to know, thank you for the hint! I think it's fine that we insert the Limit at the beginning, so no need to open a PR to change it

Copy link
Contributor

Choose a reason for hiding this comment

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

Placing SetComputeUnitLimit at the front is also fine.

I noticed that we lack test cases to ensure that the AdvanceNonceAccount instruction is always at the beginning.

I would be happy to provide a PR to move SetComputeUnitLimit to the end of the instructions and update the existing test cases, so we wouldn't need to write new ones to ensure that AdvanceNonceAccount is always at the beginning.

What do you think?

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.

4 participants