Throw errors on unknown Operation parameters

[msfeldstein]

Describe the bug Currently when you're creating operations for a transaction, you can pass incorrect parameters and the sdk quietly lets you. For example

StellarSdk.Operation.changeTrust({
    amount: "0"
  })

seems to work, but the correct params are

StellarSdk.Operation.changeTrust({
    limit: "0"
  })

We should throw an error when you try to pass amount since that's clearly something wrong. It will save developers a lot of frustration to find out early that something is wrong than to have to figure out why the operation didn't do what they expected.

[ire-and-curses]

We just did this in the Go SDK, for reference: https://github.com/stellar/go/issues/1041

[abuiles]

@msfeldstein we have this validation in place already via TypeScript. The downside is that it won't be done if you are using plain JS which is probably your scenario. While I think we could add this validations, I'd prefer we don't and rely on the tools that we are trying to double down, in this case TS.

[msfeldstein]

I'm not sure I agree with that, we shouldn't prescribe tooling to our users to that degree. If we do we should call this ts-stellar-sdk

On Fri, Sep 13, 2019, 7:10 AM Adolfo Builes [email protected] wrote:

@msfeldstein https://github.com/msfeldstein we have this validation in place already via TypeScript. The downside is that it won't be done if you are using plain JS which is probably your scenario. While I think we could add this validations, I'd prefer we don't and rely on the tools that we are trying to double down, in this case TS.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stellar/js-stellar-sdk/issues/437?email_source=notifications&email_token=AABHK34EYMQA5QZYCWMQFI3QJONOJA5CNFSM4IWHVIRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6VEH7I#issuecomment-531252221, or mute the thread https://github.com/notifications/unsubscribe-auth/AABHK3YH4SB4BPZUK7IISULQJONOJANCNFSM4IWHVIRA .

[msfeldstein]

Does ts offer any way to validate at runtime?

On Fri, Sep 13, 2019, 7:22 AM Michael Feldstein [email protected] wrote:

I'm not sure I agree with that, we shouldn't prescribe tooling to our users to that degree. If we do we should call this ts-stellar-sdk

On Fri, Sep 13, 2019, 7:10 AM Adolfo Builes [email protected] wrote:

@msfeldstein https://github.com/msfeldstein we have this validation in place already via TypeScript. The downside is that it won't be done if you are using plain JS which is probably your scenario. While I think we could add this validations, I'd prefer we don't and rely on the tools that we are trying to double down, in this case TS.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stellar/js-stellar-sdk/issues/437?email_source=notifications&email_token=AABHK34EYMQA5QZYCWMQFI3QJONOJA5CNFSM4IWHVIRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6VEH7I#issuecomment-531252221, or mute the thread https://github.com/notifications/unsubscribe-auth/AABHK3YH4SB4BPZUK7IISULQJONOJANCNFSM4IWHVIRA .

[msfeldstein]

do you have any thoughts as to why my VSCode wouldn't warn on this? It usually has good typechecking

[abuiles]

I'm not sure I agree with that, we shouldn't prescribe tooling to our users to that degree. If we do we should call this ts-stellar-sdk

You are right. I'm surprised it didn't fail for you since we have a validation in place in the code for the required fields. What was your error?

Does ts offer any way to validate at runtime?

AFAIK no, it's static.

do you have any thoughts as to why my VSCode wouldn't warn on this? It usually has good typechecking

Are you running TS? I think it will warn you on TS, but it won't in Node/Vanilla JS

[abuiles]

I transferred this issue to js-stellar-base which is where the underlying issue exists. After looking at the code, we do validate the arguments, however, we ignore extra arguments in options, this is a common pattern in JS, receive an object and take only the stuff that you need, that's how we achieve optional arguments too.

The requirement here is for the app to fail because you pass stuff which is not supposed to be in the options, I think this is a nice to have but not necessarily a bug, by looking at the documentation, it says what the params should be https://stellar.github.io/js-stellar-sdk/Operation.html#.changeTrust

Leaving it open for reference and we can fix it later.

[msfeldstein]

yeah seems like a low priority feature request.

[charlie-wasp]

@abuiles @msfeldstein is this feature request still relevant? I believe, I could take that

[abuiles]

@charlie-wasp sure thing, please go ahead!

Btw, I dropped the ball in the ts migration, maybe we should revisit it in a couple of weeks.