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

feat: add support for compute budget instructions #24086

Merged
merged 3 commits into from
Apr 26, 2022

Conversation

philcchen
Copy link
Contributor

Problem

Missing ComputeBudgetInstruction in web3 sdk

Summary of Changes

Adds ComputeBudgetInstruction to web3 sdk

@mergify mergify bot added the community Community contribution label Apr 3, 2022
@mergify mergify bot requested a review from a team April 3, 2022 07:20
RequestUnits: {
index: 0,
layout: BufferLayout.struct<ComputeBudgetInstructionInputData['RequestUnits']>([
BufferLayout.u8('instruction'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think because the ComputeBudgetInstructions use borsh rather than bincode for serialization, the instruction buffer is only a u8 rather than a u32 compared to other instructions?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that's correct!


static requestUnits(params: RequestUnitsParams): TransactionInstruction {
const type = COMPUTE_BUDGET_INSTRUCTION_LAYOUTS.RequestUnits;
const data = encodeData(type, params);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR still uses the BufferLayout to match the other programs in the sdk, but it is also possible to switch to using the borsh style serialization/deserialization i.e. what PublicKey does

Copy link
Member

Choose a reason for hiding this comment

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

This approach is fine

'ComputeBudget111111111111111111111111111111',
);

static requestUnits(params: RequestUnitsParams): TransactionInstruction {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

returning a TransactionInstruction here rather than a Transaction, the StakeProgram seemed to return primarily Transaction, but think TransactionInstruction is a bit more composable


use(chaiAsPromised);

describe.skip('ComputeBudget', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently skipping these tests since I don't think that the transaction wide compute cap is activated on most networks yet?

Copy link
Member

Choose a reason for hiding this comment

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

It's enabled by default in new runs of the test validator

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 16, 2022
@stale stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 26, 2022
@jstarry jstarry changed the title Add ComputeBudgetInstruction to web3 sdk feat: add support for compute budget instructions Apr 26, 2022
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #24086 (2c1570d) into master (2da4e3e) will decrease coverage by 11.7%.
The diff coverage is n/a.

❗ Current head 2c1570d differs from pull request most recent head 09848b3. Consider uploading reports for the commit 09848b3 to get more accurate results

@@             Coverage Diff             @@
##           master   #24086       +/-   ##
===========================================
- Coverage    81.8%    70.0%    -11.8%     
===========================================
  Files         581       37      -544     
  Lines      158312     2301   -156011     
  Branches        0      325      +325     
===========================================
- Hits       129518     1613   -127905     
+ Misses      28794      573    -28221     
- Partials        0      115      +115     

@jstarry jstarry merged commit 6bbfef7 into solana-labs:master Apr 26, 2022
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Apr 27, 2022
* Add ComputeBudgetInstruction to web3 sdk

* Prettier fix

* Rename to ComputeBudgetProgram and enable tests

Co-authored-by: Justin Starry <[email protected]>
@jstarry
Copy link
Member

jstarry commented May 10, 2022

@philcchen FYI we will unfortunately be breaking the API for the changes you added in this new pending change: #25104

@philcchen
Copy link
Contributor Author

@jstarry No worries! thanks for the heads up :)

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
* Add ComputeBudgetInstruction to web3 sdk

* Prettier fix

* Rename to ComputeBudgetProgram and enable tests

Co-authored-by: Justin Starry <[email protected]>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
* Add ComputeBudgetInstruction to web3 sdk

* Prettier fix

* Rename to ComputeBudgetProgram and enable tests

Co-authored-by: Justin Starry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants