fix(staking): report minimum deposit when commitment would decrease - #5571
fix(staking): report minimum deposit when commitment would decrease#5571sbackend123 wants to merge 7 commits into
Conversation
Pre-check manageStake against the non-decreasing commitment rule so small deposits fail with HTTP 400 and the required amount instead of a misleading gas error. Expose the minimum on GET /stake. Co-authored-by: Cursor <cursoragent@cursor.com>
| func calculateMinDeposit(potential, committed *big.Int, price uint32, height uint8) *big.Int { | ||
| minAdd := big.NewInt(1) | ||
|
|
||
| // The contract applies the minimum stake floor only when creating a stake. |
There was a problem hiding this comment.
The staking contract applies the initial minimum based on:
_addAmount < MIN_STAKE * 2 ** _height && _stakingSet == 0That is, it checks lastUpdatedBlockNumber == 0, not whether potentialStake == 0.
A possible state is:
- A stake was previously created, so lastUpdatedBlockNumber != 0.
- The effective/committed stake later becomes zero.
- The potential stake is withdrawn, leaving potentialStake == 0.
- A subsequent deposit is made.
In that state, the contract does not apply the initial MIN_STAKE * 2^height floor, but calculateMinDeposit reports that floor anyway. The API can therefore overstate the required minimum and reject deposits that the contract would accept.
The calculation needs access to the stake’s lastUpdatedBlockNumber or an equivalent “stake has previously been initialized” value. The current getStake helper only returns committed and potential stake.
Suggested fix:
- Return lastUpdatedBlockNumber from getStake.
- Pass an isInitialized/stakingSet boolean into calculateMinDeposit.
- Mirror the contract’s _stakingSet == 0 condition exactly.
A test should cover potentialStake == 0 with lastUpdatedBlockNumber != 0.
There was a problem hiding this comment.
Looks like mix of different scenarios to me, wrote in masttermost for further discussion
| } | ||
|
|
||
| stakingContract := staking.New(overlayEthAddress, stakingContractAddress, abiutil.MustParseABI(chainCfg.StakingABI), bzzTokenAddress, transactionService, common.BytesToHash(nonce), contractGasLimit, uint8(o.ReserveCapacityDoubling)) | ||
| stakingContractABI := abiutil.MustParseABI(chainCfg.StakingABI) |
There was a problem hiding this comment.
L1227-L1235
I see a bunch of issues here. This staking contract initialization is leaking unnecessary details. Now pkg/node is responsible for calling a staking-package helper to discover an internal dependency and passing it back to the staking constructor.
If staking contract changes its oracle discovery logic tomorrow, we will have to change pkg/node as well. If chainEnabled is false, we are passing zero address to staking constructor which may end up using this in GetMinDeposit func call.
I see two options:
- Either the constructor should call this function and return error OR
- We do lazy oracle resolution. i.e we resolve the oracle contract in GetMinDeposit when it needs the current price.
I would prefer the second option.
Second issue is along with chainEnabled we have storage incentives enabled check as well. We could have nodes that enable chain but not storage incentives. So it is better to do this lazily inside staking contract when GetMinDeposit is called.
| // User already has committed stake. | ||
| // Commitment protection: required = committed * price * 2^height | ||
| required := new(big.Int).SetUint64(uint64(price)) | ||
| required.Lsh(required, uint(height)) // * 2^height |
There was a problem hiding this comment.
nit: although this will work as is, I would usually prefer assigning the result into a new variable or the same variable. According to me it is clearer and reduces maintenance risk. This is the style we usually followed in the codebase in general.
For eg. you can look at pkg/accounting/accounting.go:245, :251, and :257
# Conflicts: # openapi/Swarm.yaml
|
|
||
| // getPriceOracleAddress resolves the price oracle from the staking contract on first use. | ||
| func (c *contract) getPriceOracleAddress(ctx context.Context) (common.Address, error) { | ||
| if (c.priceOracleAddress != common.Address{}) { |
There was a problem hiding this comment.
This should be protected against concurrent access.
2 participants
Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure