Skip to content

Commit

Permalink
Revise owner flush algorithm, test
Browse files Browse the repository at this point in the history
  • Loading branch information
@alnoki
alnoki committed Jun 1, 2023
1 parent 9bad2c9 commit b8be9d1
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 42 deletions.
44 changes: 25 additions & 19 deletions aptos-move/framework/aptos-framework/doc/multisig_account.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2695,17 +2695,17 @@ Add new owners, remove owners to remove, update signatures required.
owners_to_remove: <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;<b>address</b>&gt;,
optional_new_num_signatures_required: Option&lt;u64&gt;,
) <b>acquires</b> <a href="multisig_account.md#0x1_multisig_account_MultisigAccount">MultisigAccount</a> {
<a href="multisig_account.md#0x1_multisig_account_assert_multisig_account_exists">assert_multisig_account_exists</a>(multisig_address);
<b>let</b> multisig_account_ref_mut =
<b>borrow_global_mut</b>&lt;<a href="multisig_account.md#0x1_multisig_account_MultisigAccount">MultisigAccount</a>&gt;(multisig_address);
// Verify no overlap between new owners and owners <b>to</b> remove.
<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_for_each_ref">vector::for_each_ref</a>(&new_owners, |new_owner_ref| {
<b>assert</b>!(
!<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_contains">vector::contains</a>(&owners_to_remove, new_owner_ref),
<a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_argument">error::invalid_argument</a>(<a href="multisig_account.md#0x1_multisig_account_EOWNERS_TO_REMOVE_NEW_OWNERS_OVERLAP">EOWNERS_TO_REMOVE_NEW_OWNERS_OVERLAP</a>)
)
});
<a href="multisig_account.md#0x1_multisig_account_assert_multisig_account_exists">assert_multisig_account_exists</a>(multisig_address);
<b>let</b> multisig_account_ref_mut =
<b>borrow_global_mut</b>&lt;<a href="multisig_account.md#0x1_multisig_account_MultisigAccount">MultisigAccount</a>&gt;(multisig_address);
// If new owners provided, add them and emit an <a href="event.md#0x1_event">event</a>.
// If new owners provided, try <b>to</b> add them and emit an <a href="event.md#0x1_event">event</a>.
<b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_length">vector::length</a>(&new_owners) &gt; 0) {
<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_append">vector::append</a>(&<b>mut</b> multisig_account_ref_mut.owners, new_owners);
<a href="multisig_account.md#0x1_multisig_account_validate_owners">validate_owners</a>(
Expand All @@ -2717,7 +2717,7 @@ Add new owners, remove owners to remove, update signatures required.
<a href="multisig_account.md#0x1_multisig_account_AddOwnersEvent">AddOwnersEvent</a> { owners_added: new_owners }
);
};
// If owners <b>to</b> remove provided, remove them and emit an <a href="event.md#0x1_event">event</a>.
// If owners <b>to</b> remove provided, try <b>to</b> remove them.
<b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_length">vector::length</a>(&owners_to_remove) &gt; 0) {
<b>let</b> owners_ref_mut = &<b>mut</b> multisig_account_ref_mut.owners;
<b>let</b> owners_removed = <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>[];
Expand All @@ -2729,12 +2729,15 @@ Add new owners, remove owners to remove, update signatures required.
<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_swap_remove">vector::swap_remove</a>(owners_ref_mut, index)
);
});
emit_event(
&<b>mut</b> multisig_account_ref_mut.remove_owners_events,
<a href="multisig_account.md#0x1_multisig_account_RemoveOwnersEvent">RemoveOwnersEvent</a> { owners_removed }
);
// Only emit <a href="event.md#0x1_event">event</a> <b>if</b> owner(s) actually removed.
<b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_length">vector::length</a>(&owners_removed) &gt; 0) {
emit_event(
&<b>mut</b> multisig_account_ref_mut.remove_owners_events,
<a href="multisig_account.md#0x1_multisig_account_RemoveOwnersEvent">RemoveOwnersEvent</a> { owners_removed }
);
}
};
// If new signature count provided, <b>update</b> count and emit <a href="event.md#0x1_event">event</a>.
// If new signature count provided, try <b>to</b> <b>update</b> count.
<b>if</b> (<a href="../../aptos-stdlib/../move-stdlib/doc/option.md#0x1_option_is_some">option::is_some</a>(&optional_new_num_signatures_required)) {
<b>let</b> new_num_signatures_required =
<a href="../../aptos-stdlib/../move-stdlib/doc/option.md#0x1_option_extract">option::extract</a>(&<b>mut</b> optional_new_num_signatures_required);
Expand All @@ -2744,15 +2747,18 @@ Add new owners, remove owners to remove, update signatures required.
);
<b>let</b> old_num_signatures_required =
multisig_account_ref_mut.num_signatures_required;
multisig_account_ref_mut.num_signatures_required =
new_num_signatures_required;
emit_event(
&<b>mut</b> multisig_account_ref_mut.update_signature_required_events,
<a href="multisig_account.md#0x1_multisig_account_UpdateSignaturesRequiredEvent">UpdateSignaturesRequiredEvent</a> {
old_num_signatures_required,
new_num_signatures_required,
}
);
// Only <b>apply</b> <b>update</b> and emit <a href="event.md#0x1_event">event</a> <b>if</b> a change indicated.
<b>if</b> (new_num_signatures_required != old_num_signatures_required) {
multisig_account_ref_mut.num_signatures_required =
new_num_signatures_required;
emit_event(
&<b>mut</b> multisig_account_ref_mut.update_signature_required_events,
<a href="multisig_account.md#0x1_multisig_account_UpdateSignaturesRequiredEvent">UpdateSignaturesRequiredEvent</a> {
old_num_signatures_required,
new_num_signatures_required,
}
);
}
};
// Verify number of owners.
<b>let</b> num_owners = <a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector_length">vector::length</a>(&multisig_account_ref_mut.owners);
Expand Down
77 changes: 54 additions & 23 deletions aptos-move/framework/aptos-framework/sources/multisig_account.move
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1080,17 +1080,17 @@ module aptos_framework::multisig_account {
owners_to_remove: vector<address>,
optional_new_num_signatures_required: Option<u64>,
) acquires MultisigAccount {
assert_multisig_account_exists(multisig_address);
let multisig_account_ref_mut =
borrow_global_mut<MultisigAccount>(multisig_address);
// Verify no overlap between new owners and owners to remove.
vector::for_each_ref(&new_owners, |new_owner_ref| {
assert!(
!vector::contains(&owners_to_remove, new_owner_ref),
error::invalid_argument(EOWNERS_TO_REMOVE_NEW_OWNERS_OVERLAP)
)
});
assert_multisig_account_exists(multisig_address);
let multisig_account_ref_mut =
borrow_global_mut<MultisigAccount>(multisig_address);
// If new owners provided, add them and emit an event.
// If new owners provided, try to add them and emit an event.
if (vector::length(&new_owners) > 0) {
vector::append(&mut multisig_account_ref_mut.owners, new_owners);
validate_owners(
Expand All @@ -1102,7 +1102,7 @@ module aptos_framework::multisig_account {
AddOwnersEvent { owners_added: new_owners }
);
};
// If owners to remove provided, remove them and emit an event.
// If owners to remove provided, try to remove them.
if (vector::length(&owners_to_remove) > 0) {
let owners_ref_mut = &mut multisig_account_ref_mut.owners;
let owners_removed = vector[];
Expand All @@ -1114,12 +1114,15 @@ module aptos_framework::multisig_account {
vector::swap_remove(owners_ref_mut, index)
);
});
emit_event(
&mut multisig_account_ref_mut.remove_owners_events,
RemoveOwnersEvent { owners_removed }
);
// Only emit event if owner(s) actually removed.
if (vector::length(&owners_removed) > 0) {
emit_event(
&mut multisig_account_ref_mut.remove_owners_events,
RemoveOwnersEvent { owners_removed }
);
}
};
// If new signature count provided, update count and emit event.
// If new signature count provided, try to update count.
if (option::is_some(&optional_new_num_signatures_required)) {
let new_num_signatures_required =
option::extract(&mut optional_new_num_signatures_required);
Expand All @@ -1129,15 +1132,18 @@ module aptos_framework::multisig_account {
);
let old_num_signatures_required =
multisig_account_ref_mut.num_signatures_required;
multisig_account_ref_mut.num_signatures_required =
new_num_signatures_required;
emit_event(
&mut multisig_account_ref_mut.update_signature_required_events,
UpdateSignaturesRequiredEvent {
old_num_signatures_required,
new_num_signatures_required,
}
);
// Only apply update and emit event if a change indicated.
if (new_num_signatures_required != old_num_signatures_required) {
multisig_account_ref_mut.num_signatures_required =
new_num_signatures_required;
emit_event(
&mut multisig_account_ref_mut.update_signature_required_events,
UpdateSignaturesRequiredEvent {
old_num_signatures_required,
new_num_signatures_required,
}
);
}
};
// Verify number of owners.
let num_owners = vector::length(&multisig_account_ref_mut.owners);
Expand Down Expand Up @@ -1819,10 +1825,35 @@ module aptos_framework::multisig_account {
execute_rejected_transaction(owner_3, multisig_account);
}

#[test]
#[test(
owner_1 = @0x123,
owner_2 = @0x124,
owner_3 = @0x125
)]
#[expected_failure(abort_code = 0x10012, location = Self)]
fun test_flush_owner_schema_overlap_should_fail()
acquires MultisigAccount {
flush_owner_schema(@0x1, vector[@0x1], vector[@0x1], option::none());
fun test_flush_owner_schema_overlap_should_fail(
owner_1: &signer,
owner_2: &signer,
owner_3: &signer
) acquires MultisigAccount {
setup();
let owner_1_addr = address_of(owner_1);
let owner_2_addr = address_of(owner_2);
let owner_3_addr = address_of(owner_3);
create_account(owner_1_addr);
let multisig_address = get_next_multisig_account_address(owner_1_addr);
create_with_owners(
owner_1,
vector[owner_2_addr, owner_3_addr],
2,
vector[],
vector[]
);
flush_owner_schema(
multisig_address,
vector[owner_1_addr],
vector[owner_1_addr],
option::none()
);
}
}

0 comments on commit b8be9d1

Please sign in to comment.