bancorprotocol / contracts_eos

Bancor Protocol Contracts for EOS
Other
113 stars 45 forks source link

a few `is_account` is not enforced #28

Closed DenisCarriere closed 4 years ago

DenisCarriere commented 4 years ago

Noticed a few of is_account that are "dangling" (check not being enforced)

https://github.com/bancorprotocol/contracts_eos/blob/0cb1bd049af6dbd2ae2d8fd6f45a3969ca6947d4/contracts/eos/MultiConverter/MultiConverter.cpp#L87

https://github.com/bancorprotocol/contracts_eos/blob/0cb1bd049af6dbd2ae2d8fd6f45a3969ca6947d4/contracts/eos/MultiConverter/MultiConverter.cpp#L73

Before

is_account(multi_token);

After

check( is_account(multi_token), multi_token.to_string() + " account does not exist");
ricktobacco commented 4 years ago

It's better to use check if you strictly require a custom error message, otherwise is_account by itself will also throw an error that's more generic (the basic behavior we need). Thank you so much for highlighting this though, we should probably use the check approach.

DenisCarriere commented 4 years ago

@ricktobacco I've just tested this locally for myself and is_account() only returns a boolean (true/false)

It doesn't throw any error

Try adding is_account("foo"_n); anywhere in your code and you'll see it won't throw an error

I'm using the latest CDT compiler, maybe things have changed from 1.4 or 1.5 => 1.6

$ eosio-cpp --version
eosio-cpp version 1.6.2
DenisCarriere commented 4 years ago

foo account does not exist, an error should of been raised by calling this action

image

ricktobacco commented 4 years ago

Ah interesting, in that case great catch, will update now and thanks again :) !

yudilevi commented 4 years ago

@ricktobacco are these changes in for all is_account checks?

ricktobacco commented 4 years ago

@yudilevi Yes

yudilevi commented 4 years ago

@ricktobacco awesome, thanks :)