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.

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

Origin/ft signatory impl #47

Merged
merged 11 commits into from
Sep 3, 2024
Merged

Origin/ft signatory impl #47

merged 11 commits into from
Sep 3, 2024

Conversation

mubarak23
Copy link
Contributor

No description provided.

@mubarak23 mubarak23 self-assigned this Aug 30, 2024
@mubarak23 mubarak23 requested a review from Darlington02 August 30, 2024 11:18

#[test]
#[should_panic(expected: ('Account: invalid length',))]
fn test_when_permissioned_addresses_and_permissions_not_equal() {
Copy link
Member

Choose a reason for hiding this comment

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

rename test to test_should_fail_if_unequal_permissioned_addresses_and_permissions

Copy link
Member

Choose a reason for hiding this comment

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

this is still yet to be renamed

tests/test_permissionable_component.cairo Show resolved Hide resolved
tests/test_permissionable_component.cairo Outdated Show resolved Hide resolved
tests/test_permissionable_component.cairo Outdated Show resolved Hide resolved
tests/test_permissionable_component.cairo Outdated Show resolved Hide resolved
src/components/permissionable/permissionable.cairo Outdated Show resolved Hide resolved
ref self: ComponentState<TContractState>,
permissioned_addresses: Array<ContractAddress>,
permissions: Array<bool>
) {
Copy link
Member

Choose a reason for hiding this comment

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

Was going over our implementation again and it just came to mind that we should enforce only the contract owner calling this function. else permissioned addresses might give permissions to random addresses on the owner's account.

So let's enforce an assert that checks that the caller is the account owner.

Copy link
Member

Choose a reason for hiding this comment

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

you marked this as resolved, but it's still yet to be resolved. assert that only owner can call this function

) {
// validate signer
let caller = get_caller_address();
assert(self.is_valid_signer(caller), 'Account: unauthorized');
Copy link
Member

Choose a reason for hiding this comment

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

folllowing my earlier comment on enforcing only owner to call the set_permission function, this check for valid signer becomes unnecessary and redundant. Let's take it off

Copy link
Member

Choose a reason for hiding this comment

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

this hasn't been resolved too


#[test]
#[should_panic(expected: ('Account: invalid length',))]
fn test_when_permissioned_addresses_and_permissions_not_equal() {
Copy link
Member

Choose a reason for hiding this comment

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

this is still yet to be renamed

tests/test_permissionable_component.cairo Show resolved Hide resolved
@mubarak23
Copy link
Contributor Author

@Darlington02 the signature component did not implement an interface

ISignatory,

its uses #[generate_trait] which the compiler generate the corresponding trait definition,

am trying to figure out a way to access the trait definition as dispatchers inside my test.

@Darlington02
Copy link
Member

@Darlington02 the signature component did not implement an interface

ISignatory,

its uses #[generate_trait] which the compiler generate the corresponding trait definition,

am trying to figure out a way to access the trait definition as dispatchers inside my test.

Just similar to how upgradeable component is tested. You test against the preset implementation. Checking that the implementation logic within the component stands. There’s a commented section of the account component for testing is_valid_signature you could utilize. Just copy and modify to include test for root_owner too.

Also since the implementation of is_valid_signer uses the permissions valid signer that contains other logic, if that works, then you can assume other implementation of is_valid_signer works. So test against those functions within preset: is_valid_signer and is_valid_signature.

@mubarak23
Copy link
Contributor Author

@Darlington02 the signature component did not implement an interface
ISignatory,
its uses #[generate_trait] which the compiler generate the corresponding trait definition,
am trying to figure out a way to access the trait definition as dispatchers inside my test.

Just similar to how upgradeable component is tested. You test against the preset implementation. Checking that the implementation logic within the component stands. There’s a commented section of the account component for testing is_valid_signature you could utilize. Just copy and modify to include test for root_owner too.

Also since the implementation of is_valid_signer uses the permissions valid signer that contains other logic, if that works, then you can assume other implementation of is_valid_signer works. So test against those functions within preset: is_valid_signer and is_valid_signature.

am not sure if the test i wrote follow this.

kindly review

tests/test_signatory_component.cairo Show resolved Hide resolved
ref self: ComponentState<TContractState>,
permissioned_addresses: Array<ContractAddress>,
permissions: Array<bool>
) {
Copy link
Member

Choose a reason for hiding this comment

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

you marked this as resolved, but it's still yet to be resolved. assert that only owner can call this function

) {
// validate signer
let caller = get_caller_address();
assert(self.is_valid_signer(caller), 'Account: unauthorized');
Copy link
Member

Choose a reason for hiding this comment

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

this hasn't been resolved too

}

#[test]
fn test_is_valid_signer_root_owner() {
Copy link
Member

Choose a reason for hiding this comment

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

wrong test implementation! you are meant to test for the root owner not the token owner! get the root owner by calling the _get_root_owner function on the tokenbound account and testing against that instead.

Hint: you might want to expose the _get_root_owner by adding it as an extra function to the preset account.

}

#[test]
fn test_is_valid_signature_root_owner() {
Copy link
Member

Choose a reason for hiding this comment

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

wrong test implementation! you are meant to test for the root owner not the token owner! get the root owner by calling the _get_root_owner function on the tokenbound account and testing against that instead.

Hint: you might want to expose the _get_root_owner by adding it as an extra function to the preset account.

@Darlington02 Darlington02 merged commit dea487f into v3 Sep 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants