fix: loaded account datasize - #25
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkspace Solana dependency pins are bumped (including major-version changes). SDK sizing constants are replaced with ChangesLoaded-account sizing and startup setup
Panic handler attribute update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/src/accounts.rs (1)
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote:
oracle_data_len()returnssize_of, not the serialized length.The on-chain account data is
oracle.to_bytes(), whose length can be smaller thansize_of::<Oracle<T>>()due to struct padding (e.g. arepr(C)payload ending in au32). Here this is safe because it only over-estimates the loaded size, but the name implies the serialized byte length. Worth a one-line doc note that this is the (conservative) in-memory size.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/accounts.rs` around lines 58 - 61, Update the documentation on oracle_data_len() in Oracle<T> to clarify that it returns the conservative in-memory size via core::mem::size_of::<Oracle<T>>() rather than the serialized on-chain length from oracle.to_bytes(); keep the method behavior unchanged and add a short note near the oracle_data_len symbol so callers do not mistake it for the exact encoded byte length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@surfpool.sh`:
- Around line 20-23: The startup runbook invocation in surfpool.sh is missing an
explicit manifest environment, which can leave setup without rpc_api_url and
network_id at boot. Update the surfpool start command in the startup script to
pass the localnet environment alongside the existing setup runbook flags,
keeping the change localized to the surfpool start invocation.
---
Nitpick comments:
In `@sdk/src/accounts.rs`:
- Around line 58-61: Update the documentation on oracle_data_len() in Oracle<T>
to clarify that it returns the conservative in-memory size via
core::mem::size_of::<Oracle<T>>() rather than the serialized on-chain length
from oracle.to_bytes(); keep the method behavior unchanged and add a short note
near the oracle_data_len symbol so callers do not mistake it for the exact
encoded byte length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e048c20c-dc04-4871-8308-2eff64d6f8d2
📒 Files selected for processing (9)
Cargo.tomlREADME.mdprogram/tests/tests.rssdk/Cargo.tomlsdk/src/accounts.rssdk/src/constants.rssdk/src/transaction.rssurfpool.shtxtx.yml
💤 Files with no reviewable changes (1)
- sdk/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@run-validator.sh`:
- Around line 5-6: The hard-coded PROGRAM_ID and UPGRADE_AUTHORITY values in
run-validator.sh are inconsistent with the addresses used elsewhere in the PR.
Update the validator setup to use the same program and authority identifiers
referenced by the inspection traces/examples so the validator loads under the
expected keys and stays aligned with the SDK/docs.
- Around line 19-21: The SBF build in run-validator.sh is incorrectly skipped
when SO_PATH already exists, which can leave surfpool/solana-test-validator
using a stale program binary after branch or code changes. Update the validator
startup flow to always run the cargo build-sbf step before launching the
validator, using the existing build command and identifiers like SO_PATH and
cargo build-sbf, instead of gating it on file existence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 958e45d8-e109-49f1-9dff-4c6cf60c4a55
📒 Files selected for processing (7)
.gitignoreREADME.mdrun-validator.shrunbooks/setup.txsdk/build.rssdk/src/constants.rssurfpool.sh
💤 Files with no reviewable changes (2)
- runbooks/setup.tx
- surfpool.sh
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| PROGRAM_ID="fastRQJt3nLdY3QA7n8eZ8ETEVefy56ryfUGVkfZokm" | ||
| UPGRADE_AUTHORITY="admnz5UvRa93HM5nTrxXmsJ1rw2tvXMBFGauvCgzQhE" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
These hard-coded keys do not match the rest of the PR.
The script uses fastRQ…EVefy… / admnz5…CgzQhE, while the inspection traces and examples in the PR use fastRQ…EVfey… / admnsz5…CqzQhE. Starting the validator with different addresses will load the program under keys the SDK/docs are not using.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run-validator.sh` around lines 5 - 6, The hard-coded PROGRAM_ID and
UPGRADE_AUTHORITY values in run-validator.sh are inconsistent with the addresses
used elsewhere in the PR. Update the validator setup to use the same program and
authority identifiers referenced by the inspection traces/examples so the
validator loads under the expected keys and stays aligned with the SDK/docs.
| if [ ! -f "$SO_PATH" ]; then | ||
| cargo build-sbf --tools-version "$TOOLS_VERSION" --manifest-path program/Cargo.toml | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not gate the SBF build on file existence.
After the first run, this reuses whatever target/deploy/doppler_program.so is already on disk. Branch switches or program changes can then boot the validator with a stale binary, which undermines the local surfpool/solana-test-validator coverage this PR depends on.
Suggested change
-if [ ! -f "$SO_PATH" ]; then
- cargo build-sbf --tools-version "$TOOLS_VERSION" --manifest-path program/Cargo.toml
-fi
+cargo build-sbf --tools-version "$TOOLS_VERSION" --manifest-path program/Cargo.toml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -f "$SO_PATH" ]; then | |
| cargo build-sbf --tools-version "$TOOLS_VERSION" --manifest-path program/Cargo.toml | |
| fi | |
| cargo build-sbf --tools-version "$TOOLS_VERSION" --manifest-path program/Cargo.toml |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run-validator.sh` around lines 19 - 21, The SBF build in run-validator.sh is
incorrectly skipped when SO_PATH already exists, which can leave
surfpool/solana-test-validator using a stale program binary after branch or code
changes. Update the validator startup flow to always run the cargo build-sbf
step before launching the validator, using the existing build command and
identifiers like SO_PATH and cargo build-sbf, instead of gating it on file
existence.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/src/constants.rs`:
- Around line 24-28: Add a guard around DOPPLER_BINARY_SIZE so the pinned value
is verified against the actual built doppler_program artifact size instead of
only being a comment-backed constant. Update the existing constants/tests path
to compare the value used in loaded_accounts_data_size with the size of
target/deploy/doppler_program.so, and fail fast or assert when they differ. Use
DOPPLER_BINARY_SIZE as the primary symbol to locate the check and ensure the
validation is tied to the runtime-critical loaded_accounts_data_size behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 220c5cd1-0645-4bac-91f9-f462c0cb68eb
📒 Files selected for processing (2)
run-validator.shsdk/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- run-validator.sh
| // doppler program binary size (bytes): the size of the stripped deploy artifact | ||
| // (`target/deploy/doppler_program.so`). Pinned statically to the validator-confirmed | ||
| // value; if the program is rebuilt with a different toolchain, re-measure the artifact | ||
| // and update this constant (and the `loaded_accounts_data_size` tests). | ||
| pub(crate) const DOPPLER_BINARY_SIZE: u32 = 1136; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== constant definition =="
sed -n '20,40p' sdk/src/constants.rs
echo
echo "== downstream consumers/tests =="
sed -n '1,220p' sdk/src/transaction.rs
echo
echo "== search for any existing sync/guard logic =="
rg -n --hidden \
'DOPPLER_BINARY_SIZE|doppler_program\.so|loaded_accounts_data_size|run-validator|build\.rs' \
sdk run-validator.sh README.md Cargo.toml .gitignoreRepository: blueshift-gg/doppler
Length of output: 7462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run-validator.sh =="
sed -n '1,220p' run-validator.sh
echo
echo "== README references around run-validator =="
sed -n '240,280p' README.md
echo
echo "== repo-wide search for artifact size checks =="
rg -n --hidden \
'stat -c|wc -c|target/deploy/doppler_program\.so|DOPPLER_BINARY_SIZE|loaded_accounts_data_size' \
. \
-g '!target'Repository: blueshift-gg/doppler
Length of output: 3433
Add a size check for DOPPLER_BINARY_SIZE. sdk/src/constants.rs:24-28 pins a runtime-critical assumption, but there’s no guard that compares it with the actual target/deploy/doppler_program.so size. If the artifact changes, the loaded-accounts-data-size limit can drift silently while the current tests still pass against the stale constant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/constants.rs` around lines 24 - 28, Add a guard around
DOPPLER_BINARY_SIZE so the pinned value is verified against the actual built
doppler_program artifact size instead of only being a comment-backed constant.
Update the existing constants/tests path to compare the value used in
loaded_accounts_data_size with the size of target/deploy/doppler_program.so, and
fail fast or assert when they differ. Use DOPPLER_BINARY_SIZE as the primary
symbol to locate the check and ensure the validation is tied to the
runtime-critical loaded_accounts_data_size behavior.
3 participants
Problem
The SDK's transaction Builder requested a loaded-accounts-data-size limit of only
~111 bytes, which undercounts whatSIMD-0186actually meters because it ignored the per-account 64-byte metadata and the program's~1.2 KBprogramdata account. Transactions built this way would be under-budgeted and fail on-chain.Summary of Changes
SIMD-0186formula, math should be the same for feat: web3js and kit SDK, program binary generator #24surfpoolsetupVerifications
tested locally on 2 envs:
result
solana-test-validator --reset \ --upgradeable-program fastRQJt3nLdY3QA7n8eZ8ETEVefy56ryfUGVkfZokm target/deploy/doppler_program.so admnz5UvRa93HM5nTrxXmsJ1rw2tvXMBFGauvCgzQhE \ --account admnz5UvRa93HM5nTrxXmsJ1rw2tvXMBFGauvCgzQhE examples/accounts/admin-account.json \ --account QUVF91dzXWYvE5FmFEc41JZxRDmNgx8S8P6sNDWYZiW examples/accounts/sol-usdc-price-feed-oracle.json \ --account 9bA7GPqPpZ5aLbwb8E6cKvUPM8pcHXXTqLpf5zLAqHP5 examples/accounts/sol-usdt-price-feed-oracle.json \ --account 6uQ848roY5vumz43QeQguE7xCyBSmgZbwNdJMTrs2Xhy examples/accounts/bonk-sol-price-feed-oracle.json cargo run --bin single-price-feedresult
@ChiefWoods could you double check the fix on your end?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation