fix: forbid optional params

[aatifsyed]

AFAICT, Lotus has no concept of optional parameters - they're all required, they just can occasionally be null.

I wrote all this code to handle optional content descriptors, but it's no longer needed.

[ruseinov]

Yeah, I guess you refer to the ones represented by pointers, e.g *string. Those can and should be represented as Option<_>, but indeed it seems they are all required, as there is no way for a positional argument to be optional in Go without extra special treatment. Good find!

[hanabi1224]

@aatifsyed I believe there're some RPC methods that support variable-length params, e.g. EthFeeHistory

func (a *EthModule) EthFeeHistory(ctx context.Context, p jsonrpc.RawParams) (ethtypes.EthFeeHistory, error) {
    params, err := jsonrpc.DecodeParams[ethtypes.EthFeeHistoryParams](p)
    ...
}

func (e *EthFeeHistoryParams) UnmarshalJSON(b []byte) error {
    var params []json.RawMessage
    err := json.Unmarshal(b, &params)
    if err != nil {
        return err
    }
    switch len(params) {
    case 3:
        err = json.Unmarshal(params[2], &e.RewardPercentiles)
        if err != nil {
            return err
        }
        fallthrough
    case 2:
        err = json.Unmarshal(params[1], &e.NewestBlkNum)
        if err != nil {
            return err
        }
        err = json.Unmarshal(params[0], &e.BlkCount)
        if err != nil {
            return err
        }
    default:
        return xerrors.Errorf("expected 2 or 3 params, got %d", len(params))
    }
    return nil
}

[elmattic]

@aatifsyed Fixing that issue https://github.com/ChainSafe/forest/issues/3966?

[ruseinov]

@aatifsyed I believe there're some RPC methods that support variable-length params, e.g. EthFeeHistory

func (a *EthModule) EthFeeHistory(ctx context.Context, p jsonrpc.RawParams) (ethtypes.EthFeeHistory, error) {
  params, err := jsonrpc.DecodeParams[ethtypes.EthFeeHistoryParams](p)
  ...
}

func (e *EthFeeHistoryParams) UnmarshalJSON(b []byte) error {
  var params []json.RawMessage
  err := json.Unmarshal(b, &params)
  if err != nil {
      return err
  }
  switch len(params) {
  case 3:
      err = json.Unmarshal(params[2], &e.RewardPercentiles)
      if err != nil {
          return err
      }
      fallthrough
  case 2:
      err = json.Unmarshal(params[1], &e.NewestBlkNum)
      if err != nil {
          return err
      }
      err = json.Unmarshal(params[0], &e.BlkCount)
      if err != nil {
          return err
      }
  default:
      return xerrors.Errorf("expected 2 or 3 params, got %d", len(params))
  }
  return nil
}

Haven't seen that! So I guess we at least have to search for some more instances of jsonrpc.RawParams.

[ansermino]

Another example may be GasEstimateMessageGas. Here is the impl: https://github.com/filecoin-project/lotus/blob/f7632ed11ed51154f684e68e4361a0409891f5c4/node/impl/full/gas.go#L384-L417

spec is only passed to CapGasFee, which allows for it to be nil https://github.com/filecoin-project/lotus/blob/f7632ed11ed51154f684e68e4361a0409891f5c4/chain/messagepool/messagepool.go#L212-L235

[ruseinov]

Another example may be GasEstimateMessageGas. Here is the impl: https://github.com/filecoin-project/lotus/blob/f7632ed11ed51154f684e68e4361a0409891f5c4/node/impl/full/gas.go#L384-L417

spec is only passed to CapGasFee, which allows for it to be nil https://github.com/filecoin-project/lotus/blob/f7632ed11ed51154f684e68e4361a0409891f5c4/chain/messagepool/messagepool.go#L212-L235

nullable != optional. I guess the only way to really verify that is: trying to call these endpoints omitting some/all of the nullable/unused params and see what breaks.

[hanabi1224]

We could maybe introduce another Nullable wrapper to distinguish the behaviors

[ansermino]

nullable != optional. I guess the only way to really verify that is: trying to call these endpoints omitting some/all of the nullable/unused params and see what breaks.

You are correct, but we should mindful not to conflate optional parameters in Go with optional parameters in JSON. The default behaviour in Go is to accept the JSON input and use zero values.

Does anyone know where the handlers are registered for the API in Lotus? Or where they handle the conversion from HTTP request to the corresponding types?

1000% to try this out ourselves. We should ensure to start adding examples to the spec as well.

[ruseinov]

nullable != optional. I guess the only way to really verify that is: trying to call these endpoints omitting some/all of the nullable/unused params and see what breaks.

You are correct, but we should mindful not to conflate optional parameters in Go with optional parameters in JSON. The default behaviour in Go is to accept the JSON input and use zero values.

Does anyone know where the handlers are registered for the API in Lotus? Or where they handle the conversion from HTTP request to the corresponding types?

1000% to try this out ourselves. We should ensure to start adding examples to the spec as well.

I think the best bet for us to see how the parameters are parsed and set is to just run Lotus in debug mode and put some breakpoints, then observe the call stack. I'm pretty sure this is done via reflection, but the codebase it too convoluted to just try and deduce it. If you guys need some help with it - let me know, otherwise there is a manual on Notion how to run Lotus in debug mode via GoLand (Intellij IDE family).

[aatifsyed]

Thanks for the feedback gang - lotus only handles omitted parameters for the Eth* methods - other parameters must be supplied, but may be null.

I think it might be easier to keep that way - are we specifying that in the FIP @ansermino?

[ruseinov]

Thanks for the feedback gang - lotus only handles omitted parameters for the Eth* methods - other parameters must be supplied, but may be null.

I think it might be easier to keep that way - are we specifying that in the FIP @ansermino?

In theory not having to specify null params is less traffic. If that is a concern - it could be interesting to be able to omit them.

[aatifsyed]

Hailong has a better approach in #4472