Fix isolated margin ntli encoding#150
Conversation
| return nil | ||
| } | ||
|
|
||
| func isAPIResponseTarget(result any) bool { |
There was a problem hiding this comment.
Not sure if it would change the default behavior.
There was a problem hiding this comment.
we have to be sure about changes submitted in PRs, right?
There was a problem hiding this comment.
You're right to push back — it shouldn't exist. I've removed it in 01584b9.
The problem it was papering over: executeAction has seven call sites, and only two of them (UpdateLeverage, UpdateIsolatedMargin) pass a result type that doesn't decode the status envelope. UserState is a plain struct, so {"status":"err","response":"Cannot switch leverage type with open position."} unmarshals into a zero value with err == nil — the caller sees a silent success. The other five call sites pass APIResponse[T], whose UnmarshalJSON already populates Ok/Err and whose callers check it.
So the envelope check is needed for two statically-known call sites, and isAPIResponseTarget was inferring that at runtime. Worse, matching on a type-name prefix isn't a contract: renaming APIResponse would compile fine and silently disable the guard for every caller, and any future APIResponseFoo would match by accident.
Replaced with an explicit split:
signAndPost— sign + POST, returns the raw bodyexecuteAction— unchanged behavior, for targets that decode the envelope themselvesexecuteActionChecked— runsexchangeActionErrorfirst; used only by the twoUserStatecall sites
No behavior change for the five APIResponse[T] call sites, which answers the "would it change the default behavior" question by construction rather than by argument.
I also gave exchangeActionError an exported ExchangeActionError type so callers can errors.As an exchange rejection apart from a transport or decode failure.
Happy to split the refactor into its own PR if you'd rather keep this one scoped to the ntli encoding fix.
…iant
executeAction used reflect + a type-name prefix to decide at runtime whether
the caller's result type decodes the {"status":"err"} envelope itself. All
seven call sites are statically known, so split the method instead:
- signAndPost: sign + POST, returns the raw body
- executeAction: for APIResponse[T] targets, which decode the envelope
- executeActionChecked: for targets that don't (UserState)
Matching on a type name is not a contract — renaming APIResponse would have
silently disabled the guard for every caller with no compile error.
Also give exchangeActionError a typed ExchangeActionError so library users can
errors.As a rejected action apart from a transport or decode failure.
No behavior change for the five APIResponse[T] call sites.
| // exchangeActionError reports the exchange's rejection of an action, or nil if | ||
| // the response is not a rejection. A body that does not parse as the envelope | ||
| // is left for the caller's json.Unmarshal to reject. | ||
| func exchangeActionError(resp []byte) error { |
There was a problem hiding this comment.
please rename to parseExchangeActionError
| if len(envelope.Response) > 0 { | ||
| return &ExchangeActionError{Message: string(envelope.Response)} | ||
| } | ||
| return &ExchangeActionError{Message: "exchange action failed"} |
There was a problem hiding this comment.
Let's not create structs for errors; Instead, let's use error sentinels
var (
ErrExchangeAction = errors.New("exchange action failed")
)
// and then in some other place
if len(envelope.Response) > 0 {
return fmt.Errorf("%w, %s", ErrExchangeAction, envelope.Response)
}2 participants
Pull Request
Description
Brief description of the changes in this PR.
Type of Change
Testing
make testand all checks passCode Quality
Generated Code
make generateand committed any generated filesBreaking Changes
If this is a breaking change, please describe the impact and migration path for existing users:
Screenshots (if applicable)
Add screenshots to help explain your changes.
Additional Notes
Any additional information that would be helpful for reviewers.