Access Controller manifest classifications

[GhenadieVP]

• Entity Securify classification
• AC recovery classification
• AC Cancel timed recovery classification
• AC Confirm timed recovery classification

[BusyWritingCode]

Since we can rely more on the TypedManifestNativeInvocation type we can collapse this entire function into a single matches! statement:

matches!(
    context,
    InstructionContext::InvocationInstruction {
        instruction,
        typed_native_invocation: Some(
            TypedNativeInvocation {
                // Fee payment methods
                invocation: TypedManifestNativeInvocation::AccountBlueprintInvocation(AccountBlueprintInvocation::Method(AccountBlueprintMethod::LockFee(..)))
                | TypedManifestNativeInvocation::AccountBlueprintInvocation(AccountBlueprintInvocation::Method(AccountBlueprintMethod::LockContingentFee(..)))
                | TypedManifestNativeInvocation::AccessControllerBlueprintInvocation(AccessControllerBlueprintInvocation::Method(AccessControllerBlueprintMethod::LockRecoveryFee(..)))
                // Recovery methods
                | TypedManifestNativeInvocation::AccessControllerBlueprintInvocation(AccessControllerBlueprintInvocation::Method(AccessControllerBlueprintMethod::TimedConfirmRecovery(..))),
                ..
            }
        ),
        ..
    }
    // Only call-method invocations are permitted.
    if instruction.belongs_to_invocation_instructions_and(|instruction| instruction.is_call_method_instruction())
);

[BusyWritingCode]

Not sure if this should be allowed when confirming a timed recovery?

[BusyWritingCode]

Ditto: consider using the syntax I provided in the previous comment for a simpler way to express what instructions are permitted and what isn't.

[GhenadieVP]

In my understanding this is used for the scenario when the fee payer is a securified account, for which case it is needed to create the proof for locking the fee from the account

[BusyWritingCode]

Fair enough, then we should keep it. Consider using the syntax that I mentioned.

[BusyWritingCode]

There's some inconsistency between this method and the process_instruction method.

In the process_instruction method you process the instructions which initiate recovery, but not the instructions that confirm, timed confirm, or cancel the recovery proposal. So there's a disconnect between the instructions permitted in this analyzer and the instructions processed by this analyzer which is mostly incorrect.

[BusyWritingCode]

Why is there no ACCESS_CONTROLLER_TIMED_CONFIRM_RECOVERY_IDENT?

[GhenadieVP]

I'll follow the spec doc you have shared, I am not entirely sure if many of these are needed.

[BusyWritingCode]

I think that it would be better to do that since it covers all of the methods that should be covered.

[BusyWritingCode]

Here you process recovery initiation but doesn't process recovery confirmation but you permit recovery confirmation in this analyzer which is incorrect.

What's the purpose of this analyzer? To catch all ACs for which we started recovery? To catch all ACs for which we touch any recovery operation? To catch all ACs for which we confirm recovery? If we be more specific about it's purpose then we can agree on how to implement it best.

[GhenadieVP]

What's the purpose of this analyzer? To catch all ACs for which we started recovery?

Yes. This might analyzer might be a bit specific to Wallet needs, where we do care only to determine the ac for which the recovery is started. Down the line, the Wallet might not even sign this specific manifest, but a different variant based on the user signature flow.

[BusyWritingCode]

Yes. This might analyzer might be a bit specific to Wallet needs, where we do care only to determine the ac for which the recovery is started.

If you only want to determine the ACs for which recovery has started, then do you assume that in some of these manifests users will also complete the recovery? Because this would then determine what set of instructions should be permitted or not permitted.

[BusyWritingCode]

The analyzer is called StopTimedRecovery but we permit these methods. Why? Also, we permit them but don't process them, why?

[GhenadieVP]

it is allowed to perform the stop and cancel the proposal in the same transaction, instead of having separate transaction for stopping the timed recovery and then for cancelling the proposal.
it is not processed given that we do extract the necessary information from the StopTimedRecovery, but I guess there should be at least a validation that stop and cancel refer to the same AC.

[BusyWritingCode]

The typed invocation is already available in the context. You don't need to re-create it again.

[BusyWritingCode]

All of the logic in this function can be more succinctly expressed as:

matches!(
    context,
    InstructionContext::InvocationInstruction {
        typed_native_invocation: Some(TypedNativeInvocation {
            invocation:
                TypedManifestNativeInvocation::AccountBlueprintInvocation(
                    AccountBlueprintInvocation::Method(
                        AccountBlueprintMethod::Securify(..)
                        | AccountBlueprintMethod::CreateProofOfAmount(..)
                        | AccountBlueprintMethod::CreateProofOfNonFungibles(..)
                        | AccountBlueprintMethod::LockFee(..)
                        | AccountBlueprintMethod::LockContingentFee(..)
                    )
                )
                | TypedManifestNativeInvocation::IdentityBlueprintInvocation(
                    IdentityBlueprintInvocation::Method(
                        IdentityBlueprintMethod::Securify(..)
                    )
                )
                | TypedManifestNativeInvocation::AccessControllerBlueprintInvocation(
                    AccessControllerBlueprintInvocation::Function(
                        AccessControllerBlueprintFunction::Create(..)
                    )
                    | AccessControllerBlueprintInvocation::Method(
                        AccessControllerBlueprintMethod::CreateProof(..)
                    )
                )
                | TypedManifestNativeInvocation::MetadataBlueprintInvocation(
                    MetadataBlueprintInvocation::Method(
                        MetadataBlueprintMethod::Set(..)
                    )
                ),
            ..
        }),
        ..
    }
    | InstructionContext::NonInvocationInstruction {
        instruction: GroupedInstruction::ProofInstructions(..)
        | GroupedInstruction::TakeFromWorktopInstructions(..)
        | GroupedInstruction::AddressAllocationInstructions(
            AddressAllocationInstructions::AllocateGlobalAddress(
                AllocateGlobalAddress { package_address: ACCESS_CONTROLLER_PACKAGE, .. }
            )
        ),
        ..
    }
);