search

Consensys / armlet

a MythX API client wrapper
MIT License
17 stars 7 forks source link

Throw just a message on missing auth params #73

Closed daniyarchambylov closed 5 years ago

daniyarchambylov commented 5 years ago

Fixes https://github.com/ConsenSys/truffle-security/issues/72

rocky commented 5 years ago

Just out of curiosity, why is this needed? Looks like by default eslint doesn't allow throwing string literals, why do we need to go against this standard behaviour? Can't this be solved on truffle-security side?

I don't totally understand what you are asking. If you'd sketch out this in more detail maybe with code fragments, I am sure I would understand better.

Here are the characteristics I think are useful and to keep in mind.

Since the tracebacks are useless, and that has been noted in the issue that motivated this, they should not be show. Personally, I think it better not to generate something that will never be used. That is simpler.

Doing work that would need to be done by all clients of armlet should be done here. So no it is not just truffle-security that would have to change, but also vscode-solidity and ... We can have another library in between that both of these use, but that is again more complicated. I think better to address this here.

By the way, I am less concerned about standards for standards sake when they don' t make sense in the particular context that they come up.

fgimenez commented 5 years ago

@rocky it is considered good practice to throw an Error object instead of a string literal, these changes require to disable a check in eslint https://github.com/ConsenSys/armlet/pull/73/files#diff-df39304d828831c44a2b9f38cd45289cR9; for reference this is the check that these changes are disabling https://eslint.org/docs/rules/no-throw-literal

You can always extract the error message from the thrown error object using the message property, no need to show any traceback, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_objects/Error/message#Throwing_a_custom_error. In order to improve interoperability IMO it's better to stick to standards when possible, as i see it what is uncommon is the behaviour of truffle-security and vs-code integration, expecting a string from a throw, WDYT?

rocky commented 5 years ago

I think here go against the additional complexity. Use standards when they make sense, and when they don't don't use them. Right now this is a lot of additional work.

The first time someone uses armlet and describes the need for this error object then that is when we should do this. On the other hand I and others have noted that the stack trace is not useful - ever. So again adding additional complexity to make eslint happy is weird.

We do indicate though using the standard disable comment that we know about this, and are deliberately ignoring this. So in this sense we are following a standard - to say we don't think the standard applies here :-)

In science sometimes when you find you are bucking convention, it is found that the convention is wrong, and then that changes and becomes the new convention.