eosnetworkfoundation / eos-system-contracts

Other
28 stars 20 forks source link

Removed Deferred Transaction Support from System Contract #86

Closed ericpassmore closed 11 months ago

ericpassmore commented 1 year ago

System Contract

Remove need_deferred_trx from system contract. Along with updates to delegate_bandwidth.cpp and name_bidding.cpp

https://github.com/eosnetworkfoundation/eos-system-contracts/blob/717537db0a5a30461719be916871a2b23b2ca876/contracts/eosio.system/src/delegate_bandwidth.cpp#L324-L335

https://github.com/eosnetworkfoundation/eos-system-contracts/blob/717537db0a5a30461719be916871a2b23b2ca876/contracts/eosio.system/src/name_bidding.cpp#L52-L60

ericpassmore commented 1 year ago

Wrap Documentation

Need advice on how to update the Wrap Documentation. Is it still valid to have 0 values for ref_block and ref_block_prefix in the unified transaction proposed to the msig contract?

https://github.com/eosnetworkfoundation/eos-system-contracts/blob/717537db0a5a30461719be916871a2b23b2ca876/docs/04_guides/07_how-to-use-eosio.wrap.md?plain=1#L175-L205

arhag commented 1 year ago

@ericpassmore: Yes, ref_block_num and ref_block_prefix fields can remain zero.

The wrap contract does not care about what is in the transaction header other than the expiration. In fact, we may consider the interface for the exec action that takes a transaction to be misleading to the user. It is simply taking a list of actions to send inline.

It does enforce the expiration has not yet been reached, but there is really no point for us to add that check into exec. It also enforces that context_free_actions are empty to not mislead the user into thinking those will run. And yet, it doesn't enforce the value of other fields like ref_block_num, ref_block_prefix, max_net_usage_words, max_cpu_usage_ms, delay_sec even though they have particular meanings. It also does not validate that transaction_extensions are empty even though in theory those extensions could radically change the behavior of regular input transactions in the future.

The multisig contract is in a similar situation. It does enforce that contract_free_actions are empty, that the expiration has not yet been reached (which makes more sense for the multisig to enforce since there is a delay between proposal and execution unlike with the wrap contract), and it does have special rules regarding delay_sec which does change its behavior. But the other fields (ref_block_num, ref_block_prefix, max_net_usage_words, max_cpu_usage_ms, and transaction_extensions) are not validated or used in any way.

Ideally, the multisig contract's propose action interface would also be changed to just take a list of actions, an expiration, and maybe a delay_sec. (Though I also believe that delay_sec should not be provided during transaction proposal time and should instead be delayed until exec which provides more flexibility. See: https://github.com/AntelopeIO/reference-contracts/issues/25.)

Anyway, as the code exists right now, I think it is best that we just continue to document that all those unvalidated fields be either 0 or empty (depending on what is appropriate for the type).

ericpassmore commented 1 year ago

Tests for name bidding are failing. Trying to figure out how all of these is intended to work.

 // start bids
   bidname( "bob",  "prefa", core_sym::from_string("1.0003") );
   BOOST_REQUIRE_EQUAL( core_sym::from_string( "9998.9997" ), get_balance("bob") );
   bidname( "bob",  "prefb", core_sym::from_string("1.0000") );
   bidname( "bob",  "prefc", core_sym::from_string("1.0000") );
   BOOST_REQUIRE_EQUAL( core_sym::from_string( "9996.9997" ), get_balance("bob") );
   BOOST_REQUIRE_EQUAL( core_sym::from_string( "10000.0000" ), get_balance("alice") );

   // alice outbids bob on prefb
   {
      const asset initial_names_balance = get_balance("eosio.names"_n);
      // as of Leap v5 no longer support deferred trx, bob's balance only changes when winning a bid
      // previously deferred trx would debit balance then refund when canceled
      const asset bob_balance_in_auction = get_balance("bob") ;
      BOOST_REQUIRE_EQUAL( core_sym::from_string( "10000.0000" ), get_balance("alice") );
      BOOST_REQUIRE_EQUAL( success(),
                           bidname( "alice", "prefb", core_sym::from_string("1.1001") ) );

      // This fails: bob's balance should show refund of 1.0000 bid on prefb
      BOOST_REQUIRE_EQUAL( bob_balance_in_auction + core_sym::from_string("1.0000"), get_balance("bob") );
      // alice balance less bid
      BOOST_REQUIRE_EQUAL( core_sym::from_string("10000.0000") - core_sym::from_string("1.1001") , get_balance("alice") );
      // This fails: eosio.names balance should be higher by alice's successful bid
      BOOST_REQUIRE_EQUAL( initial_names_balance + core_sym::from_string("1.1001") , get_balance("eosio.names"_n) );
   }
ericpassmore commented 1 year ago

Solved it with help. Needed to explicitly refund transactions. Use bidrefund action for resource refunds and refund action for bids

// refund carls's failed bid on prefd
      BOOST_REQUIRE_EQUAL( success(), push_action( "carl"_n, "bidrefund"_n, mvo()("bidder","carl")("newname", "prefd") ) );
BOOST_REQUIRE_EQUAL( success(), push_action( "carol1111111"_n, "refund"_n, mvo()("owner", "carol1111111") ) );