Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use constructor to construct action instead of assigning members after the fact. #951

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use constructor to construct action instead of assigning members after the fact. #951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
816535a
Use constructor to construct `action` instead of assigning members af…
greg7mdp Oct 18, 2024
986562e
Change `account` and `name` from `action_base` to be const.
greg7mdp Oct 18, 2024
e57cc43
Revert "Change `account` and `name` from `action_base` to be const."
greg7mdp Oct 18, 2024
c569d7e
restore two original comments.
greg7mdp Oct 18, 2024
97254ff
Merge branch 'main' of github.com:AntelopeIO/spring into const_action
greg7mdp Oct 18, 2024
b158e5b
Updated the `eosio_wasm_spec_test_generator` to use constructor synta…
greg7mdp Oct 18, 2024
84dc44c
Merge branch 'main' of github.com:AntelopeIO/spring into const_action
greg7mdp Oct 18, 2024
ad8ed8a
Merge branch 'main' of github.com:AntelopeIO/spring into const_action
greg7mdp Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4711,11 +4711,8 @@ struct controller_impl {
*/
signed_transaction get_on_block_transaction()
{
action on_block_act;
on_block_act.account = config::system_account_name;
on_block_act.name = "onblock"_n;
on_block_act.authorization = vector<permission_level>{{config::system_account_name, config::active_name}};
on_block_act.data = fc::raw::pack(chain_head.header());
action on_block_act(vector<permission_level>{{config::system_account_name, config::active_name}},
config::system_account_name, "onblock"_n, fc::raw::pack(chain_head.header()));

signed_transaction trx;
trx.actions.emplace_back(std::move(on_block_act));
Expand Down
13 changes: 5 additions & 8 deletions unittests/api_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,14 +963,11 @@ void push_trx(Tester& test, T ac, uint32_t billed_cpu_time_us , uint32_t max_cpu
bool explicit_bill, std::vector<char> payload = {}, name account = "testapi"_n, transaction_metadata::trx_type trx_type = transaction_metadata::trx_type::input ) {
signed_transaction trx;

action act;
act.account = ac.get_account();
act.name = ac.get_name();
if ( trx_type != transaction_metadata::trx_type::read_only ) {
auto pl = vector<permission_level>{{account, config::active_name}};
act.authorization = pl;
}
act.data = payload;
auto pl = vector<permission_level>{};
if ( trx_type != transaction_metadata::trx_type::read_only )
pl = vector<permission_level>{{account, config::active_name}};

action act(std::move(pl), ac.get_account(), ac.get_name(), payload);

trx.actions.push_back(act);
test.set_transaction_headers(trx);
Expand Down
2 changes: 1 addition & 1 deletion unittests/block_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( block_with_invalid_tx_mroot_test, T, testers )
auto signed_tx = packed_trx.get_signed_transaction();

// Change the transaction that will be run
signed_tx.actions[0].name = "something"_n;
const_cast<action_name&>(signed_tx.actions[0].name) = "something"_n;
// Re-sign the transaction
signed_tx.signatures.clear();
signed_tx.sign(main.get_private_key(config::system_account_name, "active"), main.get_chain_id());
Expand Down
45 changes: 13 additions & 32 deletions unittests/currency_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,8 @@ class currency_tester : public T {
auto push_action(const account_name& signer, const action_name &name, const variant_object &data ) {
string action_type_name = abi_ser.get_action_type(name);

action act;
act.account = "eosio.token"_n;
act.name = name;
act.authorization = vector<permission_level>{{signer, config::active_name}};
act.data = abi_ser.variant_to_binary(action_type_name, data, abi_serializer::create_yield_function( T::abi_serializer_max_time ));
action act(vector<permission_level>{{signer, config::active_name}}, "eosio.token"_n, name,
abi_ser.variant_to_binary(action_type_name, data, abi_serializer::create_yield_function( T::abi_serializer_max_time )));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the original way of initializing individual fields explicitly clearer and more readable. action is a data class and used most locally. It is probably appropriate to be a struct without const.

Copy link
Contributor Author

@greg7mdp greg7mdp Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much disagree. Because action has constructors, it implies that the object should be constructed using the provided constructors.

The direct member initialization forgoes any hope to ensure that objects are always constructed in a consistent fashion, and that some internal members (possibly dependent on the constructors parameters) are always initialized correctly. This is not the case for action, but I feel it is a better pattern to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arhag do you have an opinion on whether it is worthwhile to merge this PR?

signed_transaction trx;
trx.actions.emplace_back(std::move(act));
Expand Down Expand Up @@ -422,15 +419,9 @@ BOOST_FIXTURE_TEST_CASE( test_proxy_deferred, pre_disable_deferred_trx_currency_
// set up proxy owner
{
signed_transaction trx;
action setowner_act;
setowner_act.account = "proxy"_n;
setowner_act.name = "setowner"_n;
setowner_act.authorization = vector<permission_level>{{"proxy"_n, config::active_name}};
setowner_act.data = proxy_abi_ser.variant_to_binary("setowner", mutable_variant_object()
("owner", "alice")
("delay", 10),
abi_serializer::create_yield_function( abi_serializer_max_time )
);
action setowner_act(vector<permission_level>{{"proxy"_n, config::active_name}}, "proxy"_n, "setowner"_n,
proxy_abi_ser.variant_to_binary("setowner", mutable_variant_object()("owner", "alice")("delay", 10),
abi_serializer::create_yield_function( abi_serializer_max_time )));
trx.actions.emplace_back(std::move(setowner_act));

set_transaction_headers(trx);
Expand Down Expand Up @@ -478,15 +469,9 @@ BOOST_FIXTURE_TEST_CASE( test_deferred_failure, pre_disable_deferred_trx_currenc
// set up proxy owner
{
signed_transaction trx;
action setowner_act;
setowner_act.account = "proxy"_n;
setowner_act.name = "setowner"_n;
setowner_act.authorization = vector<permission_level>{{"proxy"_n, config::active_name}};
setowner_act.data = proxy_abi_ser.variant_to_binary("setowner", mutable_variant_object()
("owner", "bob")
("delay", 10),
abi_serializer::create_yield_function( abi_serializer_max_time )
);
action setowner_act(vector<permission_level>{{"proxy"_n, config::active_name}}, "proxy"_n, "setowner"_n,
proxy_abi_ser.variant_to_binary("setowner", mutable_variant_object()("owner", "bob")("delay", 10),
abi_serializer::create_yield_function( abi_serializer_max_time )));
trx.actions.emplace_back(std::move(setowner_act));

set_transaction_headers(trx);
Expand Down Expand Up @@ -529,15 +514,11 @@ BOOST_FIXTURE_TEST_CASE( test_deferred_failure, pre_disable_deferred_trx_currenc
// set up alice owner
{
signed_transaction trx;
action setowner_act;
setowner_act.account = "bob"_n;
setowner_act.name = "setowner"_n;
setowner_act.authorization = vector<permission_level>{{"bob"_n, config::active_name}};
setowner_act.data = proxy_abi_ser.variant_to_binary("setowner", mutable_variant_object()
("owner", "alice")
("delay", 0),
abi_serializer::create_yield_function( abi_serializer_max_time )
);

action setowner_act(vector<permission_level>{{"bob"_n, config::active_name}},
"bob"_n, "setowner"_n,
proxy_abi_ser.variant_to_binary("setowner", mutable_variant_object()("owner", "alice")("delay", 0),
abi_serializer::create_yield_function(abi_serializer_max_time)));
trx.actions.emplace_back(std::move(setowner_act));

set_transaction_headers(trx);
Expand Down
7 changes: 1 addition & 6 deletions unittests/dry_run_trx_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,9 @@ struct dry_run_trx_tester : T {

auto send_db_api_transaction(action_name name, bytes data, const vector<permission_level>& auth={{"alice"_n, config::active_name}},
transaction_metadata::trx_type type=transaction_metadata::trx_type::input, uint32_t delay_sec=0) {
action act;
action act(auth, "noauthtable"_n, name, data);
signed_transaction trx;

act.account = "noauthtable"_n;
act.name = name;
act.authorization = auth;
act.data = data;

trx.actions.push_back( act );
T::set_transaction_headers( trx );
trx.delay_sec = delay_sec;
Expand Down
7 changes: 3 additions & 4 deletions unittests/eosio.token_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,9 @@ class eosio_token_tester : public T {
T::action_result push_action( const account_name& signer, const action_name &name, const variant_object &data ) {
string action_type_name = abi_ser.get_action_type(name);

action act;
act.account = "eosio.token"_n;
act.name = name;
act.data = abi_ser.variant_to_binary( action_type_name, data, abi_serializer::create_yield_function( T::abi_serializer_max_time ) );
action act({}, "eosio.token"_n, name,
abi_ser.variant_to_binary( action_type_name, data,
abi_serializer::create_yield_function( T::abi_serializer_max_time )));

return base_tester::push_action( std::move(act), signer.to_uint64_t() );
}
Expand Down
28 changes: 6 additions & 22 deletions unittests/protocol_feature_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,13 +879,8 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(only_bill_to_first_authorizer, T, testers) { try {
chain.produce_block();

{
action act;
act.account = tester_account;
act.name = "null"_n;
act.authorization = vector<permission_level>{
{tester_account, config::active_name},
{tester_account2, config::active_name}
};
action act(vector<permission_level>{{tester_account, config::active_name},
{tester_account2, config::active_name}}, tester_account, "null"_n, {});

signed_transaction trx;
trx.actions.emplace_back(std::move(act));
Expand Down Expand Up @@ -924,13 +919,8 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(only_bill_to_first_authorizer, T, testers) { try {
chain.produce_block();

{
action act;
act.account = tester_account;
act.name = "null2"_n;
act.authorization = vector<permission_level>{
{tester_account, config::active_name},
{tester_account2, config::active_name}
};
action act(vector<permission_level>{{tester_account, config::active_name},
{tester_account2, config::active_name}}, tester_account, "null2"_n, {});

signed_transaction trx;
trx.actions.emplace_back(std::move(act));
Expand Down Expand Up @@ -1452,10 +1442,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(webauthn_recover_key, T, testers) { try {
c.produce_block();

signed_transaction trx;
action act;
act.account = "bob"_n;
act.name = ""_n;
act.authorization = vector<permission_level>{{"bob"_n,config::active_name}};
action act(vector<permission_level>{{"bob"_n,config::active_name}}, "bob"_n, ""_n, {});
trx.actions.push_back(act);

c.set_transaction_headers(trx);
Expand Down Expand Up @@ -1500,10 +1487,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(webauthn_assert_recover_key, T, testers) { try {
c.produce_block();

signed_transaction trx;
action act;
act.account = "bob"_n;
act.name = ""_n;
act.authorization = vector<permission_level>{{"bob"_n,config::active_name}};
action act(vector<permission_level>{{"bob"_n,config::active_name}}, "bob"_n, ""_n, {});
trx.actions.push_back(act);

c.set_transaction_headers(trx);
Expand Down
7 changes: 1 addition & 6 deletions unittests/read_only_trx_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,9 @@ struct read_only_trx_tester : T {
}

auto send_db_api_transaction( action_name name, bytes data, const vector<permission_level>& auth={{"alice"_n, config::active_name}}, transaction_metadata::trx_type type=transaction_metadata::trx_type::input, uint32_t delay_sec=0 ) {
action act;
action act(auth, "noauthtable"_n, name, data);
signed_transaction trx;

act.account = "noauthtable"_n;
act.name = name;
act.authorization = auth;
act.data = data;

trx.actions.push_back( act );
T::set_transaction_headers( trx );
trx.delay_sec = delay_sec;
Expand Down
5 changes: 1 addition & 4 deletions unittests/snapshot_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,10 +672,7 @@ void jumbo_row_test()
chain.produce_block();

signed_transaction trx;
action act;
act.account = "jumbo"_n;
act.name = "jumbo"_n;
act.authorization = vector<permission_level>{{"jumbo"_n,config::active_name}};
action act(vector<permission_level>{{"jumbo"_n,config::active_name}}, "jumbo"_n, "jumbo"_n, {});
trx.actions.push_back(act);

chain.set_transaction_headers(trx);
Expand Down
40 changes: 8 additions & 32 deletions unittests/wasm-spec-tests/generated-tests/address.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ BOOST_DATA_TEST_CASE(address_0_check_throw, boost::unit_test::data::xrange(0,11)
tester.set_code("wasmtest"_n, wasm_address_0);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we found no way to regenerate these files properly? I think if anything should update the generator,

spring/unittests/wasm-spec-tests/generator/eosio_wasm_spec_test_generator.cpp

Lines 39 to 43 in f45de1f

func << " action test;\n";
func << " test.account = \"wasmtest\"_n;\n";
func << " test.name = account_name((uint64_t)index);\n";
func << " test.authorization = {{\"wasmtest\"_n, config::active_name}};\n\n";
func << " push_action(tester, std::move(test), \"wasmtest\"_n.to_uint64_t());\n";

btw it sure looks like AntelopeIO/wasm-spec-tests@026fb4b found a way to regenerate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the generator b158e5b.


BOOST_CHECK_THROW(push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t()), wasm_execution_error);
tester.produce_block();
Expand All @@ -28,10 +25,7 @@ BOOST_DATA_TEST_CASE(address_0_pass, boost::unit_test::data::xrange(11,12), inde
tester.set_code("wasmtest"_n, wasm_address_0);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t());
tester.produce_block();
Expand All @@ -49,10 +43,7 @@ BOOST_DATA_TEST_CASE(address_2_check_throw, boost::unit_test::data::xrange(0,15)
tester.set_code("wasmtest"_n, wasm_address_2);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

BOOST_CHECK_THROW(push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t()), wasm_execution_error);
tester.produce_block();
Expand All @@ -66,10 +57,7 @@ BOOST_DATA_TEST_CASE(address_2_pass, boost::unit_test::data::xrange(15,17), inde
tester.set_code("wasmtest"_n, wasm_address_2);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t());
tester.produce_block();
Expand All @@ -87,10 +75,7 @@ BOOST_DATA_TEST_CASE(address_3_check_throw, boost::unit_test::data::xrange(0,3),
tester.set_code("wasmtest"_n, wasm_address_3);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

BOOST_CHECK_THROW(push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t()), wasm_execution_error);
tester.produce_block();
Expand All @@ -104,10 +89,7 @@ BOOST_DATA_TEST_CASE(address_3_pass, boost::unit_test::data::xrange(3,4), index)
tester.set_code("wasmtest"_n, wasm_address_3);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t());
tester.produce_block();
Expand All @@ -125,10 +107,7 @@ BOOST_DATA_TEST_CASE(address_4_check_throw, boost::unit_test::data::xrange(0,3),
tester.set_code("wasmtest"_n, wasm_address_4);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

BOOST_CHECK_THROW(push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t()), wasm_execution_error);
tester.produce_block();
Expand All @@ -142,10 +121,7 @@ BOOST_DATA_TEST_CASE(address_4_pass, boost::unit_test::data::xrange(3,4), index)
tester.set_code("wasmtest"_n, wasm_address_4);
tester.produce_block();

action test;
test.account = "wasmtest"_n;
test.name = account_name((uint64_t)index);
test.authorization = {{"wasmtest"_n, config::active_name}};
action test({{"wasmtest"_n, config::active_name}}, "wasmtest"_n, account_name((uint64_t)index), {});

push_action(tester, std::move(test), "wasmtest"_n.to_uint64_t());
tester.produce_block();
Expand Down
Loading