Add fail_reason to proposal_object #1036
Conversation
There was a problem hiding this comment.
@jmjatlanta it seems you misunderstood the intention of the linked issue.
|
|
||
| bool is_authorized_to_execute(database& db)const; | ||
| bool is_authorized_to_execute(database& db) const; | ||
| bool is_authorized_to_execute(database& db, std::string& fail_reason) const; |
There was a problem hiding this comment.
This looks confusing.
const here means the function won't change anything, but passing a non-const reference into the function means it actually will change something, actually most of time the reference is to the member variable in the object.
There was a problem hiding this comment.
I was unaware that some think that a const member function is unable to modify anything. I'm sorry that it would be confusing to some. This change will probably be backed out due to your "misunderstood intention" comment above, but I'll just make this comment to educate others:
A const member function means that member variables of the object will not be modified by the call. Variables that are not part of the object can be modified, and you can be fairly sure they will be if you're passing in a non-const reference or a non-const pointer.
| if ( proposal.is_authorized_to_execute(d, proposal.fail_reason) | ||
| && proposal.review_period_time && proposal.fail_reason.empty() ) | ||
| { | ||
| proposal.fail_reason = "Awaiting expiration of review period"; |
There was a problem hiding this comment.
proposal is a const reference, able to be updated?
BTW I think this won't trigger before BSIP 39 is implemented, even if temp-account is involved.
There was a problem hiding this comment.
proposal is a non-const reference passed in to to the lambda. But once again, this will probably be backed out because of the misunderstanding of #730.
| @@ -103,6 +103,11 @@ object_id_type proposal_create_evaluator::do_apply(const proposal_create_operati | |||
| std::set_difference(required_active.begin(), required_active.end(), | |||
| proposal.required_owner_approvals.begin(), proposal.required_owner_approvals.end(), | |||
| std::inserter(proposal.required_active_approvals, proposal.required_active_approvals.begin())); | |||
| if ( proposal.is_authorized_to_execute(d, proposal.fail_reason) | |||
| && proposal.review_period_time && proposal.fail_reason.empty() ) | |||
There was a problem hiding this comment.
&& proposal.review_period_time &&: I always think it's better to explicitly check with .valid() to avoid potential issues, but I may be wrong.
There was a problem hiding this comment.
Yes, I'm not a fan of "a non-zero value is a false" either. valid() shows the intent much more clearly. But once again, will probably be backed out due to my misunderstanding of #730.
| { | ||
| execution_authorized = false; | ||
| if(p.fail_reason.empty()) | ||
| p.fail_reason = "Awaiting expiration of review period"; |
There was a problem hiding this comment.
I think this will likely mess up things, E.G. it may make the proposal look like will succeed to execute on expiration, but it can still fail due to insufficient balance or fee. I'd recommend leaving the reason empty in this case, let the UI/client software decide what to show.
There was a problem hiding this comment.
I will be rolling back all the changes to this method due to my misunderstanding of #730.
| }); | ||
|
|
||
| // If the proposal has a review period, don't bother attempting to authorize/execute it. | ||
| // Proposals with a review period may never be executed except at their expiration. | ||
| if( _proposal->review_period_time ) | ||
| return void_result(); | ||
|
|
||
| if( _proposal->is_authorized_to_execute(d) ) | ||
| if( execution_authorized ) | ||
| { | ||
| // All required approvals are satisfied. Execute! | ||
| _executed_proposal = true; |
There was a problem hiding this comment.
Actually what I specifically wanted for this issue is to update the reason in catch below.
libraries/chain/proposal_object.cpp
Outdated
| @@ -43,8 +49,7 @@ bool proposal_object::is_authorized_to_execute(database& db) const | |||
| } | |||
| catch ( const fc::exception& e ) | |||
| { | |||
| //idump((available_active_approvals)); | |||
| //wlog((e.to_detail_string())); | |||
| fail_reason = e.to_string(fc::log_level(fc::log_level::all)); | |||
There was a problem hiding this comment.
Don't update reason here, but let UI/client decide.
|
Ok, so let me make sure I understand the intent of #730.
Am I correct? |
|
@jmjatlanta you got it. What we're looking for is that IF we were "authorized to execute" and the review period was |
Rolled back changes due to misunderstanding issue, added code to solve issue. Added test to prove solution works.
tests/tests/proposal_tests.cpp
Outdated
| { | ||
| try | ||
| { | ||
| // create an account for Bob |
There was a problem hiding this comment.
Why not use the ACTORS macro here?
There was a problem hiding this comment.
I was having trouble, as I was not familiar with proposals. So to have complete control, I did this, copying another test. I'll change it back to using the ACTORS macro.
tests/tests/proposal_tests.cpp
Outdated
| @@ -0,0 +1,116 @@ | |||
| /* | |||
There was a problem hiding this comment.
While it is generally a good idea to put some structure into our testsuite, I think this particular file only adds confusion, because we already have a lot of tests around proposals and most of them are not in this file. I suggest to move this single test elsewhere.
There was a problem hiding this comment.
The other file that contains a number of proposal tests is authority_tests.cpp. It seemed to me that they had a lot to do with committee stuff, so I didn't put it there. I'll move it there, and avoid this extra file.
tests/tests/proposal_tests.cpp
Outdated
| processed_transaction processed = PUSH_TX( db, trx ); | ||
| prop = db.get<proposal_object>(processed.operation_results.front().get<object_id_type>()); | ||
| } | ||
| catch( fc::exception& ex ) |
There was a problem hiding this comment.
No need to catch this, an unexpected exception will cause the test to fail anyway.
There was a problem hiding this comment.
Once again, leftover crumbs from learning about proposals. I will clean it.
tests/tests/proposal_tests.cpp
Outdated
| try { | ||
| db.get<proposal_object>(prop.id); | ||
| } | ||
| catch (fc::exception& ex) |
There was a problem hiding this comment.
No need to catch this, an unexpected exception will cause the test to fail anyway.
There was a problem hiding this comment.
Fixed in next check-in
tests/tests/proposal_tests.cpp
Outdated
|
|
||
| // check fail reason | ||
| const proposal_object& result = db.get<proposal_object>(prop.id); | ||
| BOOST_CHECK_EQUAL(result.fail_reason, "Assert Exception: insufficient_balance: Insufficient Balance: " |
There was a problem hiding this comment.
Please relax this test, I think it is sufficient to test for substring "Assert Exception". The precise error message for this reason is not the scope of this test.
There was a problem hiding this comment.
Fixed in next check in.
tests/tests/proposal_tests.cpp
Outdated
| "10 BTS, unable to transfer '20 BTS' from account 'bob' to 'alice' Unable to transfer 20 BTS " | ||
| "from bob to alice"); | ||
| } | ||
| catch (fc::exception& ex) |
There was a problem hiding this comment.
Please use FC_LOG_AND_RETHROW macro instead.
There was a problem hiding this comment.
Fixed in next check in.
| } | ||
| } | ||
| */ | ||
| // NOTE: All authority checks happen outside of evaluators |
There was a problem hiding this comment.
I think this comment can be removed completely.
There was a problem hiding this comment.
Fixed in next check in
| @@ -59,7 +59,7 @@ BOOST_AUTO_TEST_CASE( simple_single_signature ) | |||
| sign(trx, nathan_key); | |||
| PUSH_TX( db, trx, database::skip_transaction_dupe_check ); | |||
|
|
|||
| BOOST_CHECK_EQUAL(get_balance(nathan, core), old_balance - 500); | |||
| BOOST_CHECK_EQUAL(get_balance(nathan, core), static_cast<int64_t>(old_balance - 500)); | |||
There was a problem hiding this comment.
Do all these (unrelated) casts fix compiler warnings, or what is the reason?
There was a problem hiding this comment.
Yes, compiler warnings.
If I understand the code correctly, there are 2 ways that the transactions in a proposal may get applied:
In the case of the second option, would you expect to see
Update: Oops, just remembered that option 1 does not apply here (processing at expiration), as the proposal is deleted at expiration. So please ignore that. But my question as to block number still applies. |
|
Just think out loud.. I don't think a block number have much value in the error message, perhaps a timestamp is better. Perhaps even better to have the error details in a json so it will be easier for UI to parse. Anyway, current implementation is good enough to me, we can roll it out and see if there is feedback. |
PR for Issue #730
Added fail_reason to the proposal_object. This text gets updated during the creation or update of the proposal object.