Add tests to REST client for simulate and generate_only
Closed this issue · 13 comments
This issue is a reproduction of the issue cosmos/gaia#335 filed by @feri42
Summary of Bug
Calling the Gaia-Lite for Cosmos API calls:
POST /staking/delegators/{delegatorAddr}/delegations
POST /staking/delegators/{delegatorAddr}/unbonding_delegations
POST /staking/delegators/{delegatorAddr}/redelegations
with simulate=true or with the query paramter generate_only=true returns a Internal Server Error with the following response body:
{
"jsonrpc": "2.0",
"id": "",
"error": {
"code": -32603,
"message": "Internal error",
"data": "runtime error: invalid memory address or nil pointer dereference"
}
}Version
Gaiacli and gaiad version is 2.0.7
Steps to Reproduce
Unpack the attached body.json file and run:
curl -X POST -H "Content-Type: application/json" https://<gaiad>/staking/delegators/cosmos1j4heycy25ersvld236f3ckpn9avjjt4p3tmg23/delegations -d @body.json
For Admin Use
- Not duplicate issue
- Appropriate labels applied
- Appropriate contributors tagged
- Contributor assigned/self-assigned
I can check this one.
The problem is that amount arrives nil:
cosmos-sdk/x/staking/types/msg.go
Line 187 in 53bf227
The problem is the request, it has:
"delegation": {
"amount": "10000",
"denom": "uatom"
}
but should be:
"amount": {
"amount": "10000",
"denom": "uatom"
}
Created a test to show it in https://github.com/cosmos/cosmos-sdk/pull/5906/files
Thank you for looking into this @jgimeno ! I've notified the original poster of the issue, probably good to have tests for this regardless though?
@okwme I think part of the problem comes with the Type Int
type Int struct {
i *big.Int
}
In this case it happens that the request does not find the field but sets the default
Amount sdk.Coin `json:"amount" yaml:"amount"`
Coin has
type Coin struct {
Denom string `protobuf:"bytes,1,opt,name=denom,proto3" json:"denom,omitempty"`
Amount Int `protobuf:"bytes,2,opt,name=amount,proto3,customtype=Int" json:"amount"`
}
Which the default Int has a nil pointer inside as default value. So I think having the option to set an Int{} that the default value is nil and all the methods fail maybe needs a rethink. Thoughts? :D @okwme @alexanderbez @alessio
What's the issue exactly? The Int type should have nothing to do with this AFAIK. Seems like a poorly
constructed DelegateRequest? Or is there an actual bug?
Well was just a suggestion, the problem is the msgValidate:
func (msg MsgDelegate) ValidateBasic() error {
if msg.DelegatorAddress.Empty() {
return ErrEmptyDelegatorAddr
}
if msg.ValidatorAddress.Empty() {
return ErrEmptyValidatorAddr
}
if !msg.Amount.Amount.IsPositive() {
return ErrBadDelegationAmount
}
return nil
}
on a bad constructed request it checks if it is positive, but msg.Amount.Amount is nil and it panics.
Amount.Amount exists, but the inner i is nil. So I dont know if this is a good design on Int. I cannot check just if Amount.Amount.i is nil. Do you know what I mean? The problem is the panic.
We already have checks like this elsewhere. The check should be:
if msg.Amount.IsNil() || !msg.Amount.Amount.IsPositive() {
return ErrBadDelegationAmount
}@alessio did this with the x/distribution params validation helpers. I recommend we go through all the param validation functions and REST handlers to enforce this 👍
ah ok! Thanks @alexanderbez I was checking the Int for Nil and did not see that Amount has it!
@jgimeno Thank you very much for looking into this. I can confirm that is the case with
POST /staking/delegators/{delegatorAddr}/delegations
POST /staking/delegators/{delegatorAddr}/unbonding_delegations
I have been relying on the documentation at https://cosmos.network/rpc/#/ , which is incorrect. May I also point out that I did not find specs on how to construct transactions, so I resorted to guessing (incorrectly).