Repository navigation
fix(factory): add create_pool boundary validations 47-50 - #596
Merged
ritaifeoluwa merged 13 commits intoOct 22, 2026
Merged
ritaifeoluwa merged 13 commits into
ritaifeoluwa merged 13 commits into
Conversation
Reject clearly out-of-range create_pool inputs at the API boundary with typed FactoryErrors before any conversion or deployment work: - Validation 47: reject raw min_stake_amount < -MAX_STAKE_AMOUNT before the non-positive defaulting path, so a corrupt magnitude such as i128::MIN is refused instead of silently rewritten to the dust default. - Validation 48: reject a staking asset equal to the pool admin address, which would leave the pool administered by its own token contract and unable to authorize admin actions. - Validation 49: require the SEP-41 balance entry point in addition to symbol when probing the staking asset, so half-implemented token interfaces cannot be registered as pool stake assets. - Validation 50: reject an empty create_pools_batch request with a typed EmptyBatch error instead of consuming admin auth while creating nothing.
Add a SymbolOnlyToken mock that answers symbol but not balance, and one regression test per new boundary check: - test_create_pool_rejects_min_stake_amount_below_negative_max - test_create_pool_rejects_i128_min_min_stake_amount - test_create_pool_rejects_asset_equal_to_admin - test_create_pool_rejects_asset_without_balance_entry_point - test_create_pools_batch_rejects_empty_batch Each asserts the typed FactoryError and that pool_count stays 0.
Pre-existing formatting drift in farming-pool, vesting-wallet, and the factory integration-test stubs caused `cargo fmt --all -- --check` to fail on main. Apply rustfmt so the CI formatting gate passes.
✅ Deploy Preview for sdcontracts ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@KidDev88 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…_rejects_asset_equal_to_admin.1.json
…s_batch_rejects_empty_batch.1.json
…_rejects_asset_without_balance_entry_point.1.json
…_rejects_i128_min_min_stake_amount.1.json
…_rejects_min_stake_amount_below_negative_max.1.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #540, Closes #541, Closes #542, Closes #543
Type of Change
Summary
create_pool/create_pools_batchstill accepted several clearly invalid requests at the API boundary: an absurd negativemin_stake_amountmagnitude was silently rewritten to the dust default, a pool could be created with its own staking token as admin, a contract answeringsymbolbut notbalancepassed asset validation, and an empty batch consumed admin auth while creating nothing. This PR adds four fail-fast boundary checks (validations 47–50) so each case returns a typedFactoryErrorbefore any conversion or deployment work.Motivation / Context
Closes #540, Closes #541, Closes #542, Closes #543
These issues report a validation gap in the
create_pooloperation that can lead to invalid pool creation and put user funds at risk. Invalid input should be refused at the API boundary with a typed error rather than surviving into helper defaulting logic or pool deployment.Changes
soroban/contracts/factory/src/lib.rsmin_stake_amount < -MAX_STAKE_AMOUNTbefore the non-positive defaulting logic, soi128::MINand similar corrupt magnitudes are refused rather than silently becomingMIN_STAKE_AMOUNTasset == admin— a pool administered by its own staking token cannot authorize pause/upgrade/multiplier changesvalidate_assetnow also probes SEP-41balance, so a contract that answers onlysymbolcannot be registered as a stake assetcreate_pools_batchwith a typedEmptyBatcherrorsoroban/contracts/factory/src/types.rs: addFactoryError::EmptyBatch = 20soroban/contracts/factory/src/test.rs:SymbolOnlyTokenmock (symbolwithoutbalance) and one regression test per new checkcargo fmt --all -- --checkpassesTesting
cargo test -p factory --lib→ 103 passed, 0 failed (98 pre-existing + 5 new)pool_count == 0):test_create_pool_rejects_min_stake_amount_below_negative_maxtest_create_pool_rejects_i128_min_min_stake_amounttest_create_pool_rejects_asset_equal_to_admintest_create_pool_rejects_asset_without_balance_entry_pointtest_create_pools_batch_rejects_empty_batchcargo clippy -p factory --lib -- -D warnings— cleanTradeoffs
FactoryErrorvariants (InvalidMinStakeAmount,InvalidAsset) except forEmptyBatch, keeping the on-chain error surface stable where possible.min_stake_amountatMAX_STAKE_AMOUNT, so small negatives like-5still map to the documented dust default whilei128::MINis rejected.transfer_adminpath: the publiccreate_poolentry points already validate the stored admin, but the sharedcreate_pool_innerboundary now refuses the asset/admin collision explicitly.Out of scope
mainunrelated to these validations. Tests were verified against awasm-opt -Ozbuild of the same sources.soroban/contracts/factory/tests/still do not compile (noted in fix(factory): add create_pool boundary validations 43-46 #590) and are unrelated to this change.MAX_STAKE_AMOUNT,MIN_LOCK_PERIOD,MAX_CREDIT_RATE, …).