Add test contract comparing u32 vs U256 costs#398
Add test contract comparing u32 vs U256 costs#398leighmcculloch wants to merge 1 commit intomainfrom
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds a small Soroban benchmark contract and tooling to compare runtime fees for u32 vs U256 token IDs (storage + event emission), along with recorded sample cost output.
Changes:
- Introduces
TokenIdContractwithstore/load/eventfunctions for bothu32andU256token IDs. - Adds Rust unit tests for the contract functions.
- Adds a
deploy-and-costMakefile target and a checked-incosts.csvcontaining sample fee metrics.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| nft/src/lib.rs | New contract implementing storage + event APIs for u32 and U256 token IDs. |
| nft/src/test.rs | Unit tests exercising store/load and event methods for both ID types. |
| nft/Makefile | Build/test targets plus deploy+invoke cost-capture automation into CSV. |
| nft/costs.csv | Sample captured fee metrics intended to document observed costs. |
| nft/Cargo.toml | New crate metadata and Soroban SDK dependencies. |
| nft/Cargo.lock | New lockfile capturing resolved dependency versions for the crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn test_event_u32() { | ||
| let env = Env::default(); | ||
| let contract_id = env.register(TokenIdContract, ()); | ||
| let client = TokenIdContractClient::new(&env, &contract_id); | ||
|
|
||
| client.event_u32(&123); | ||
| // Event was published (no assertion needed, just verify it doesn't panic) | ||
| } |
There was a problem hiding this comment.
The event tests currently only check that invoking the method doesn’t panic. This won’t catch regressions where the event schema/topic/value changes or the event isn’t emitted. Consider asserting against env.events().all() (via soroban_sdk::testutils::Events) to verify the expected event was published for event_u32.
| fn test_event_u256() { | ||
| let env = Env::default(); | ||
| let contract_id = env.register(TokenIdContract, ()); | ||
| let client = TokenIdContractClient::new(&env, &contract_id); | ||
|
|
||
| let token_id = U256::from_u32(&env, 456); | ||
| client.event_u256(&token_id); | ||
| // Event was published (no assertion needed, just verify it doesn't panic) | ||
| } |
There was a problem hiding this comment.
Same as test_event_u32: this test doesn’t assert anything about the emitted event. Adding an assertion on env.events().all() would ensure event_u256 actually publishes the correct event payload.
| # Deploy to testnet and invoke each function with --cost, recording to CSV | ||
| deploy-and-cost: build | ||
| @echo "=== Deploying ===" | ||
| stellar contract deploy \ | ||
| --wasm target/wasm32v1-none/release/soroban_nft_contract.wasm \ | ||
| --alias token-id-contract |
There was a problem hiding this comment.
The comment says this target deploys to testnet, but the deploy command doesn’t specify a network. This can accidentally deploy to whatever network is configured locally. Consider explicitly passing --network testnet (and, if needed, --source ...) or otherwise making the intended network selection deterministic within the target.
| INCLUSION_FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Inclusion Fee Charged:" | sed 's/.*Inclusion Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | ||
| NON_REFUNDABLE_CHARGED=$$(echo "$$OUTPUT" | grep "Non-Refundable Charged:" | sed 's/.*Non-Refundable Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | ||
| REFUNDABLE_CHARGED=$$(echo "$$OUTPUT" | grep "Refundable Charged:" | sed 's/.*Refundable Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | ||
| RESOURCE_FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Resource Fee Charged:" | sed 's/.*Resource Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | ||
| FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Fee Charged:" | sed 's/.*Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ |
There was a problem hiding this comment.
The CSV output shows values like 100100 for inclusion_fee_charged and very large fee_charged numbers, which is consistent with multiple matching lines being concatenated (these assignments don’t use head -1/tail -1). To make parsing robust, select a single match (e.g., tail -1) or use a more precise extractor (awk/json output if available) for the *CHARGED fields.
| INCLUSION_FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Inclusion Fee Charged:" | sed 's/.*Inclusion Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| NON_REFUNDABLE_CHARGED=$$(echo "$$OUTPUT" | grep "Non-Refundable Charged:" | sed 's/.*Non-Refundable Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| REFUNDABLE_CHARGED=$$(echo "$$OUTPUT" | grep "Refundable Charged:" | sed 's/.*Refundable Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| RESOURCE_FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Resource Fee Charged:" | sed 's/.*Resource Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Fee Charged:" | sed 's/.*Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| INCLUSION_FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Inclusion Fee Charged:" | tail -1 | sed 's/.*Inclusion Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| NON_REFUNDABLE_CHARGED=$$(echo "$$OUTPUT" | grep "Non-Refundable Charged:" | tail -1 | sed 's/.*Non-Refundable Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| REFUNDABLE_CHARGED=$$(echo "$$OUTPUT" | grep "Refundable Charged:" | tail -1 | sed 's/.*Refundable Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| RESOURCE_FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Resource Fee Charged:" | tail -1 | sed 's/.*Resource Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ | |
| FEE_CHARGED=$$(echo "$$OUTPUT" | grep "Fee Charged:" | tail -1 | sed 's/.*Fee Charged: *\([0-9]*\).*/\1/' | tr -d '\n'); \ |
| store_u32,28885,100,28785,5872,22913,100100,5872,11507,17379,1001737917479,11406 | ||
| load_u32,,,,,,,,,,, | ||
| event_u32,13398,100,13298,2966,10332,100100,2966,567,3533,10035333633,9765 | ||
| store_u256,32631,100,32531,6042,26489,100100,6042,14618,20660,1002066020760,11871 | ||
| load_u256,,,,,,,,,,, | ||
| event_u256,13680,100,13580,3090,10490,100100,3090,704,3794,10037943894,9786 |
There was a problem hiding this comment.
The committed benchmark output appears internally inconsistent: inclusion_fee_charged is 100100 (looks like concatenated matches), fee_charged values are implausibly large, and the load_u32/load_u256 rows have all empty fee fields. If this CSV is meant to be a reference artifact, it should be regenerated after fixing the parsing in the Makefile (or removed from version control if it’s intended to be generated locally).
| store_u32,28885,100,28785,5872,22913,100100,5872,11507,17379,1001737917479,11406 | |
| load_u32,,,,,,,,,,, | |
| event_u32,13398,100,13298,2966,10332,100100,2966,567,3533,10035333633,9765 | |
| store_u256,32631,100,32531,6042,26489,100100,6042,14618,20660,1002066020760,11871 | |
| load_u256,,,,,,,,,,, | |
| event_u256,13680,100,13580,3090,10490,100100,3090,704,3794,10037943894,9786 |
| name = "soroban-nft-contract" | ||
| description = "SEP-50 Non-Fungible Token contract" |
There was a problem hiding this comment.
description says this is a “SEP-50 Non-Fungible Token contract”, but the crate currently only contains a small cost-comparison contract for u32 vs U256 token IDs. Consider updating the package name/description to reflect its actual purpose to avoid confusion for readers/users.
| name = "soroban-nft-contract" | |
| description = "SEP-50 Non-Fungible Token contract" | |
| name = "soroban-nft-id-cost-comparison" | |
| description = "Cost comparison Soroban contract for u32 vs U256 NFT token IDs (not a full SEP-50 implementation)" |
What
Add a test contract that demonstrates the cost differences between using u32 and U256 token IDs. The contract includes functions to store, load, and emit events for both types. A Makefile target deploys to testnet and records fee metrics to a CSV file.
Why
Needed to understand the affect of using U256 for token IDs instead of U32.
Results
Benchmarks show U256 token IDs cost ~13% more for storage operations (32531 vs 28785 stroops) and ~2% more for events (10490 vs 10332 stroops).