Escrow support. #866

oxarbitrage · 2018-04-20T21:53:45Z

This PR introduces one object and four new operations(with corresponding validations and evaluators) to equip the bitshares-core with support for native escrow smart contracts.

The code included is an adapted version of the escrow system implemented for steem(steemit/steem#143).
It will put bitshares in the same level of steem in regards to escrow support, nothing more, nothing less however; bitshares can and should do a better job. The code presented here attempts to boost development of escrow features in graphene blockchains in general and bitshares in particular.

The code is complainant with escrow bsip 40(bitshares/bsips#76).

This is not final code and of course require hardfork. I will leave the community to decide if we will have time for reviewing and improving this for the next consensus release or if it should be included in a future one. BSIP40 needs to be voted in in order for this to be included, the code is the result of a research in bitshares new operations i made to present in the Shanghai Graphene conference that i decided to submit now.

I understand our top devs are very busy and this may not be the best time, i also understand BSIP should came first, my apologies for not taking the most formal path but i don't want this work to just die.

Limitations:

Only escrows in BTS are supported. Bitshares should support all assets to be used for escrow(mpas, uias, etc) but that will need additional code and research. More specifically i am having issues at: libraries/chain/include/graphene/chain/protocol/asset.hpp:88 with amount in an asset and fee in BTS. As mentioned, need more research, any help here is appreciated.

Improvements:

If escrow expires it will stay there until all parties approve and one of the parties execute escrow_release. We should delete expired escrows at maintenance time and return funds to starter account if no further action than the create was taken.
Need to add cli_wallet calls and api call to get escrows. This is easy, was not added as first we need to have consensus in the core code.
Finish some test cases.

Discussion:

Fees need discussion. Charging for not completed escrows may be a problem however a very tiny fee will prevent network spam.

commits in this pull should be squashed as they are messy.

abitmore

Good job.

Firstly, I think it's not the correct time to discuss or work on this in detail now, better do it after we tagged a hard fork release for testnet.

Things to be done that I've thought of:

automatically release funds and / or do other actions after reached every expiration time involved
Initial fee need to be peg to one of the existing operations on chain
hardfork guard for new operations nested inside a proposal
new / update database APIs for query
CLI commands / APIs

libraries/chain/db_notify.cpp

abitmore · 2018-04-21T07:23:50Z

libraries/chain/escrow_evaluator.cpp

+               esc.to                     = o.to;
+               esc.agent                  = o.agent;
+               esc.amount                 = o.amount;
+               esc.pending_fee            = o.agent_fee;


If put a pending fee here, I think we can't add it to agent's balance above.

abitmore · 2018-04-21T07:30:42Z

libraries/chain/db_balance.cpp

@@ -186,4 +187,13 @@ void database::deposit_witness_pay(const witness_object& wit, share_type amount)
   return;
 }

+// adding the escrow call here to dont create a new file
+// change this to where it correspond
+const escrow_object& database::get_escrow( account_id_type account, uint32_t escrow_id )const {


Perhaps better move it to db_getter.cpp.

abitmore · 2018-04-21T07:33:05Z

libraries/chain/escrow_evaluator.cpp

+
+         FC_ASSERT( o.ratification_deadline > db().head_block_time() );
+         FC_ASSERT( o.escrow_expiration > db().head_block_time() );
+         FC_ASSERT( db().get_balance( o.from, o.amount.asset_id ) >= (o.amount + o.fee + o.agent_fee) );


What if amount, fee and agent_fee have different asset_id?

those checks are done in validate() at escrow.cpp however the fee==bts was not enforced. added here cb779d1 this is temporal until we can make the escrow works with other asset than bts, then some of this rules will need to change.

abitmore · 2018-04-21T07:43:01Z

libraries/chain/include/graphene/chain/database.hpp

@@ -327,6 +327,9 @@ namespace graphene { namespace chain {
         // helper to handle witness pay
         void deposit_witness_pay(const witness_object& wit, share_type amount);

+         // escrow call here
+         const escrow_object&   get_escrow( account_id_type account, uint32_t escrow_id )const;
+


Move this as well.

abitmore · 2018-04-21T07:44:16Z

libraries/chain/include/graphene/chain/escrow_evaluator.hpp

+         typedef escrow_transfer_operation operation_type;
+
+         void_result do_evaluate( const escrow_transfer_operation& o );
+         void_result do_apply( const escrow_transfer_operation& o );


Don't use void_result here when creating a new object.

abitmore · 2018-04-21T07:48:21Z

libraries/chain/protocol/escrow.cpp

+         FC_ASSERT( from != to );
+         FC_ASSERT( from != agent && to != agent );
+         FC_ASSERT( agent_fee.asset_id == amount.asset_id ); // agent fee only in bts
+         FC_ASSERT( amount.asset_id == asset_id_type()); // only bts is allowed by now


IMHO don't enforce this. This feature is more useful for bitUSD etc.

this is temporally disabled as current code don't work with other coin than core, check Limitation section of first message. I can remove, add the test case for escrow MPA and you can see the error i am referring to more in detail.
Will be great if you can take a look to that with me as i got stuck there and one of the features that will make the difference is definitely allow all assets to be in escrow.

abitmore · 2018-04-22T10:09:23Z

@oxarbitrage sorry, I didn't realize that impl object type is used here, which made the situation a bit hard to judge.

There are some differences about design philosophy between Steem and BitShares.

BitShares uses object IDs in API (e.g. get_objects) and protocol (e.g. order_create_operation returns an ID which will then needed for order_cancel_operation). So,

IDs are in object types or impl object types, between the two, impl object types are meant to not be referenced in protocol (this was done) or API (due to historical reasons 2.1.0 and etc are extensively used by client software);
BitShares uses websocket connection and object subscription mechanism: clients can subscribe to object IDs, changed objects or their IDs will be pushed from API server to clients.

Steem avoided object IDs in API and protocol. All objects have additional IDs whose values are decided by users who sign transactions, and internal IDs is hidden from API, so not visible to client software.

it's easier to have multiple operations in one transaction, e.g. create a post then vote on it;
Steem doesn't have object subscription feature, and is phasing out websocket connection (data pushing) in favor of pure HTTP connection (data polling) about API.

I'm not going to judge which one is better at this moment. Anyway, when designing, need to think about compatibility as well as ease of use.

Need more discussion.

oxarbitrage · 2018-04-23T12:36:31Z

i can change this to outside the impl. was not sure what to do at the beginning and followed the steem path but this can be changed if needed.

abitmore · 2018-04-23T16:02:16Z

@oxarbitrage best if we have more discussions on why and how.

xeroc · 2018-05-28T13:51:41Z

I like this!

If escrow expires it will stay there until all parties approve and one of the parties execute escrow_release. We should delete expired escrows at maintenance time and return funds to starter account if no further action than the create was taken.

I think this should be considered the right way of doing it. What is important to consider is if an escrow should expire if it is in dispute and if setting an escrow into dispute may require a higher fee to prevent abuse.

oxarbitrage · 2018-06-04T20:57:12Z

alfredo@alfredo-Inspiron-5559 ~/CLionProjects/escrow $ ./tests/chain_test -t escrow_tests/escrow_uia
Random number generator seeded to 1528145777
GRAPHENE_TESTING_GENESIS_TIMESTAMP is 1431700000
Running 1 test case...
3378982ms th_a       db_management.cpp:153         open                 ] Wiping object_database due to missing or wrong version
3379081ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
3379082ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
3379128ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/22ad-35f8-2521-c932 ...
3379296ms th_a       object_database.cpp:111       open                 ] Done opening object database.
3386709ms th_a       escrow_tests.cpp:1315         test_method          ] 10 assert_exception: Assert Exception
a.asset_id == b.asset_id: 
    {}
    th_a  asset.hpp:88 operator+

    {}
    th_a  evaluator.cpp:51 start_evaluate

    {"op":[49,{"fee":{"amount":0,"asset_id":"1.3.0"},"from":"1.2.16","to":"1.2.17","agent":"1.2.18","amount":{"amount":1000,"asset_id":"1.3.1"},"escrow_id":0,"agent_fee":{"amount":0,"asset_id":"1.3.1"},"json_meta":"","ratification_deadline":"2020-09-13T12:28:40","escrow_expiration":"2020-09-13T12:30:20"}]}
    th_a  db_block.cpp:642 apply_operation

    {"trx":{"ref_block_num":4,"ref_block_prefix":4241172113,"expiration":"2020-09-13T12:28:00","operations":[[49,{"fee":{"amount":0,"asset_id":"1.3.0"},"from":"1.2.16","to":"1.2.17","agent":"1.2.18","amount":{"amount":1000,"asset_id":"1.3.1"},"escrow_id":0,"agent_fee":{"amount":0,"asset_id":"1.3.1"},"json_meta":"","ratification_deadline":"2020-09-13T12:28:40","escrow_expiration":"2020-09-13T12:30:20"}]],"extensions":[],"signatures":["20479eb033192eddb0a947ea9597dc6127f313870eebe86516a1b4cdc89762cf9825c70d668f0466404b183d17485d5acdb5b14d03646e4bd3d2e29a124bd54353"]}}
    th_a  db_block.cpp:628 _apply_transaction

    {"trx":{"ref_block_num":4,"ref_block_prefix":4241172113,"expiration":"2020-09-13T12:28:00","operations":[[49,{"fee":{"amount":0,"asset_id":"1.3.0"},"from":"1.2.16","to":"1.2.17","agent":"1.2.18","amount":{"amount":1000,"asset_id":"1.3.1"},"escrow_id":0,"agent_fee":{"amount":0,"asset_id":"1.3.1"},"json_meta":"","ratification_deadline":"2020-09-13T12:28:40","escrow_expiration":"2020-09-13T12:30:20"}]],"extensions":[],"signatures":["20479eb033192eddb0a947ea9597dc6127f313870eebe86516a1b4cdc89762cf9825c70d668f0466404b183d17485d5acdb5b14d03646e4bd3d2e29a124bd54353"]}}
    th_a  db_block.cpp:227 push_transaction

    {"tx":{"ref_block_num":4,"ref_block_prefix":4241172113,"expiration":"2020-09-13T12:28:00","operations":[[49,{"fee":{"amount":0,"asset_id":"1.3.0"},"from":"1.2.16","to":"1.2.17","agent":"1.2.18","amount":{"amount":1000,"asset_id":"1.3.1"},"escrow_id":0,"agent_fee":{"amount":0,"asset_id":"1.3.1"},"json_meta":"","ratification_deadline":"2020-09-13T12:28:40","escrow_expiration":"2020-09-13T12:30:20"}]],"extensions":[],"signatures":["20479eb033192eddb0a947ea9597dc6127f313870eebe86516a1b4cdc89762cf9825c70d668f0466404b183d17485d5acdb5b14d03646e4bd3d2e29a124bd54353"]}}
    th_a  database_fixture.cpp:1174 _push_transaction
3386967ms th_a       db_management.cpp:194         close                ] Rewinding from 4 to 0
3387088ms th_a       db_management.cpp:204         close                ] Database close unexpected exception: {"code":10,"name":"assert_exception","message":"Assert Exception","stack":[{"context":{"level":"error","file":"fork_database.cpp","line":41,"method":"pop_block","hostname":"","thread_name":"th_a","timestamp":"2018-06-04T20:56:27"},"format":"_head: no blocks to pop","data":{}},{"context":{"level":"warn","file":"db_block.cpp","line":430,"method":"pop_block","hostname":"","thread_name":"th_a","timestamp":"2018-06-04T20:56:27"},"format":"","data":{}}]}
unknown location(0): fatal error in "escrow_uia": unknown type
/home/alfredo/CLionProjects/escrow/tests/common/database_fixture.cpp(251): last checkpoint

*** 1 failure detected in test suite "Master Test Suite"
alfredo@alfredo-Inspiron-5559 ~/CLionProjects/escrow $

oxarbitrage · 2018-06-04T21:18:00Z

#0  graphene::chain::operator+ (a=..., b=...) at /home/alfredo/CLionProjects/escrow/libraries/chain/include/graphene/chain/protocol/asset.hpp:88
#1  0x00000000014730d5 in graphene::chain::escrow_transfer_evaluator::do_evaluate (this=this@entry=0x7fffffffb720, o=...)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/escrow_evaluator.cpp:38
#2  0x000000000134670e in graphene::chain::evaluator<graphene::chain::escrow_transfer_evaluator>::evaluate (this=0x7fffffffb720, o=...)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/include/graphene/chain/evaluator.hpp:159
#3  0x00000000013fd46b in graphene::chain::generic_evaluator::start_evaluate (this=this@entry=0x7fffffffb720, eval_state=..., op=..., apply=<optimized out>)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/evaluator.cpp:47
#4  0x00000000011b7bfa in graphene::chain::op_evaluator_impl<graphene::chain::escrow_transfer_evaluator>::evaluate (this=<optimized out>, eval_state=..., op=..., 
    apply=<optimized out>) at /home/alfredo/CLionProjects/escrow/libraries/chain/include/graphene/chain/evaluator.hpp:134
#5  0x000000000117380a in graphene::chain::database::apply_operation (this=this@entry=0x258caa0, eval_state=..., op=...)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/db_block.cpp:639
#6  0x00000000011779f6 in graphene::chain::database::_apply_transaction (this=this@entry=0x258caa0, trx=...)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/db_block.cpp:617
#7  0x0000000001178ccf in graphene::chain::database::_push_transaction (this=this@entry=0x258caa0, trx=...)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/db_block.cpp:242
#8  0x0000000001179082 in graphene::chain::database::<lambda()>::operator() (__closure=<synthetic pointer>)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/db_block.cpp:224
#9  graphene::chain::detail::with_skip_flags<graphene::chain::database::push_transaction(const graphene::chain::signed_transaction&, uint32_t)::<lambda()> > (
    callback=..., skip_flags=0, db=...) at /home/alfredo/CLionProjects/escrow/libraries/chain/include/graphene/chain/db_with.hpp:129
#10 graphene::chain::database::push_transaction (this=this@entry=0x258caa0, trx=..., skip=skip@entry=0)
    at /home/alfredo/CLionProjects/escrow/libraries/chain/db_block.cpp:225
#11 0x000000000114b107 in graphene::chain::test::_push_transaction (db=..., tx=..., skip_flags=skip_flags@entry=0)
    at /home/alfredo/CLionProjects/escrow/tests/common/database_fixture.cpp:1171
#12 0x00000000010ae537 in escrow_tests::escrow_uia::test_method (this=this@entry=0x7fffffffccf0)
    at /home/alfredo/CLionProjects/escrow/tests/tests/escrow_tests.cpp:1310
#13 0x00000000010afa1b in escrow_tests::escrow_uia_invoker () at /home/alfredo/CLionProjects/escrow/tests/tests/escrow_tests.cpp:1280
#14 0x0000000000ba9127 in boost::unit_test::ut_detail::invoker<boost::unit_test::ut_detail::unused>::invoke<void (*)()> (this=<optimized out>, f=<optimized out>)
    at /home/alfredo/opt/boost_1_57_0/include/boost/test/utils/callback.hpp:56
#15 boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke (this=<optimized out>)
    at /home/alfredo/opt/boost_1_57_0/include/boost/test/utils/callback.hpp:89
#16 0x0000000000f7ec51 in boost::unit_test::callback0<boost::unit_test::ut_detail::unused>::operator() (this=<optimized out>)
    at /home/alfredo/opt/boost_1_57_0/include/boost/test/utils/callback.hpp:118
#17 boost::unit_test::(anonymous namespace)::zero_return_wrapper_t<boost::unit_test::callback0<boost::unit_test::ut_detail::unused> >::operator() (
    this=<optimized out>) at /home/alfredo/opt/boost_1_57_0/include/boost/test/impl/unit_test_monitor.ipp:41
#18 boost::unit_test::ut_detail::invoker<int>::invoke<boost::unit_test::(anonymous namespace)::zero_return_wrapper_t<boost::unit_test::callback0<> > > (
    this=<optimized out>, f=...) at /home/alfredo/opt/boost_1_57_0/include/boost/test/utils/callback.hpp:42
#19 boost::unit_test::ut_detail::callback0_impl_t<int, boost::unit_test::(anonymous namespace)::zero_return_wrapper_t<boost::unit_test::callback0<boost::unit_test::ut_detail::unused> > >::invoke (this=<optimized out>) at /home/alfredo/opt/boost_1_57_0/include/boost/test/utils/callback.hpp:89
#20 0x0000000000f813b1 in boost::unit_test::callback0<int>::operator() (this=0x7fffffffdc20)
    at /home/alfredo/opt/boost_1_57_0/include/boost/test/utils/callback.hpp:118
#21 boost::detail::do_invoke<boost::scoped_ptr<boost::detail::translate_exception_base>, boost::unit_test::callback0<int> > (F=..., tr=...)
    at /home/alfredo/opt/boost_1_57_0/include/boost/test/impl/execution_monitor.ipp:281

pmconrad · 2019-04-10T07:10:12Z

Lacking specification / BSIP, no progress. Closing.

oxarbitrage added 14 commits April 8, 2018 17:49

initial building

19633f0

dd get_escrow

d7a1958

add the operations code

6b867de

apply fix to escrow_release_evaluator

2ca54bd

adding escrow_approve and other fixes

dd36cdd

move escrow tests and other fixes

5cf6efc

add validation and other fixes

eb6f034

do some cleanup

c97b375

dirty validation tests

64cc2d7

apply abit patch from steem and other fixes

7686cd7

small spacing cleanup

86e6971

add hardfork

c0b5289

cleanup the tests

4d5fbc2

remove commented code

1655bb1

abitmore requested review from pmconrad, abitmore and xeroc April 21, 2018 07:18

abitmore added 2a Discussion Needed Prompt for team to discuss at next stand up. hardfork labels Apr 21, 2018

abitmore reviewed Apr 21, 2018

View reviewed changes

abitmore added this to the Future Consensus-Changing Release milestone Apr 21, 2018

oxarbitrage added 2 commits April 21, 2018 12:09

replace void_result in object creation

c4e49bd

move get_escrow to db_getter

5766384

oxarbitrage added 2 commits April 24, 2018 09:23

fee only in bts for all escrow ops (temp)

cb779d1

add code to get_relevant_accounts

1581691

oxarbitrage mentioned this pull request May 8, 2018

New BSIP: Escrow Services bitshares/bsips#44

Open

delete non disputed escrow object on expiration

0d57c9a

oxarbitrage added 3 commits May 13, 2018 20:13

hardfork proposal guard

17846ad

add escrow_api

95d3845

add cli_wallet commands

be7daf6

init uia support

e9895fe

starting support for other than core assets

e5814e6

abitmore closed this Jun 15, 2018

abitmore reopened this Jun 15, 2018

abitmore modified the milestones: Future Consensus-Changing Release, 201903 - Consensus-Changing Release Jul 30, 2018

abitmore modified the milestones: 201811 - Consensus-Upgrade Release, 201905 - Consensus-Upgrade Release Nov 12, 2018

abitmore modified the milestones: 201905 - Consensus-Upgrade Release, Future Consensus-Upgrade Release Feb 5, 2019

pmconrad closed this Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escrow support. #866

Escrow support. #866

oxarbitrage commented Apr 20, 2018

abitmore left a comment •

edited

Loading

abitmore Apr 21, 2018

abitmore Apr 21, 2018

abitmore Apr 21, 2018

oxarbitrage Apr 24, 2018 •

edited

Loading

abitmore Apr 21, 2018

abitmore Apr 21, 2018

abitmore Apr 21, 2018

oxarbitrage Apr 24, 2018

abitmore commented Apr 22, 2018

oxarbitrage commented Apr 23, 2018 •

edited

Loading

abitmore commented Apr 23, 2018

xeroc commented May 28, 2018

oxarbitrage commented Jun 4, 2018

oxarbitrage commented Jun 4, 2018

pmconrad commented Apr 10, 2019

Escrow support. #866

Escrow support. #866

Conversation

oxarbitrage commented Apr 20, 2018

abitmore left a comment • edited Loading

Choose a reason for hiding this comment

abitmore Apr 21, 2018

Choose a reason for hiding this comment

abitmore Apr 21, 2018

Choose a reason for hiding this comment

abitmore Apr 21, 2018

Choose a reason for hiding this comment

oxarbitrage Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore Apr 21, 2018

Choose a reason for hiding this comment

abitmore Apr 21, 2018

Choose a reason for hiding this comment

abitmore Apr 21, 2018

Choose a reason for hiding this comment

oxarbitrage Apr 24, 2018

Choose a reason for hiding this comment

abitmore commented Apr 22, 2018

oxarbitrage commented Apr 23, 2018 • edited Loading

abitmore commented Apr 23, 2018

xeroc commented May 28, 2018

oxarbitrage commented Jun 4, 2018

oxarbitrage commented Jun 4, 2018

pmconrad commented Apr 10, 2019

abitmore left a comment •

edited

Loading

oxarbitrage Apr 24, 2018 •

edited

Loading

oxarbitrage commented Apr 23, 2018 •

edited

Loading