Add ace to pool hook

[matYang]

No description provided.

[RensR]

This comment structure doesn't follow the style guide, but this is a completely internal project right?

[matYang]

latest version of IPolicyEngine.sol has almost finished audit and will be ported to a public repo and published in an npm package in the coming week, we'll eventually consume from there, let's keep the source as is for now

[RensR]

Unusual pragma for something with no imports

[matYang]

ditto

[RensR]

This isn't resolved? What do you mean ditto?

[RensR]

This is only used to cut off the first 4 bytes.

Can't you just add this function to AdvancedPoolHooksSetup, and maybe just make it external, make data calldata and use [4:]?

[RensR]

When you're doing this in almost every test you can probably make it part of the setup and just do it in the few tests that need another prank

[RensR]

Why is this a different contract?

[matYang]

technically setPolicyEngineAllowFailedDetach is a standalone method so warranted its own test contract, but it was so simple I was thinking maybe just include in the same file; nvm, extracting it into its own

[masha256]

I dont think we want to have this intermediate PoolHookOutboundPolicyDataV1. It would be cleaner if we just directly pass the calldata to the policy engine as the payload and have the extractor explicity decode these parameters into usable policy parameters. We also want to use tokenArgs as the "context" (ie, where something like the offchain permit can be passed to the policy engine). For example, the preflightCheck could be as simple as:

  function preflightCheck(
    Pool.LockOrBurnInV1 calldata lockOrBurnIn,
    uint16 blockConfirmationRequested,
    bytes calldata tokenArgs
  ) external {
    if (i_authorizedCallersEnabled) {
      _validateCaller();
    }
    checkAllowList(lockOrBurnIn.originalSender);

    if (address(s_policyEngine) != address(0)) {
      s_policyEngine.run(
        IPolicyEngine.Payload({selector: msg.sig, sender: msg.sender, data: msg.data[4:], context: tokenArgs})
      );
    }
  }

And postFlightCheck:

  function postFlightCheck(
    Pool.ReleaseOrMintInV1 calldata releaseOrMintIn,
    uint256 localAmount,
    uint16 blockConfirmationRequested
  ) external {
    if (i_authorizedCallersEnabled) {
      _validateCaller();
    }

    if (address(s_policyEngine) != address(0)) {
      s_policyEngine.run(
        IPolicyEngine.Payload({selector: msg.sig, sender: msg.sender, data: msg.data[4:], context: releaseOrMintIn.offchainTokenData})
      );
    }
  }

[masha256]

Do you think we really need this? We have recently updated the ACE PolicyProtected to try/catch and not revert if the detatch reverts. A policy engine should not be able to hold hostage a contract. I think its fine to do the same here and not have the extra code.

[matYang]

with a defence in depth mentality, we should be more explicit about reverts, i.e. if detach reverts, it happens for a reason, the operator should properly investigate, then explicitly ack to allow detach reverts, this allows operators to reliably catch errors, instead of things failing silently and going unnoticed

[masha256]

In PolicyProtected we do emit an event when it fails:

try s_policyEngine.detach() {
        // Detachment succeeded
      } catch (bytes memory reason) {
        emit PolicyEngineDetachFailed(address(s_policyEngine), reason);
      }

I would argue if its good enough for our client contracts (that inherit PolicyProtected) it should be good enough here. After all, we only did this approach because the pool was too big to inherit PolicyProtected, so logically they should implement the same pattern. But I will leave it to you guys - just feels like extra bytecode and complexity.

[matYang]

we probably operate with different trust assumptions, we have to assume the hook to be externally deployed and managed, and potentially using 3P policy engines, for example, if a 3P policy engine requires a prefund at time of attach, and auto-refunds at detach, letting detach revert non-blockingly can lead to loss of ability to receive refunds.

I've updated the hook to mirror PolicyProtected events, if user wants to replicate the same behavior as PolicyProtected, they can use the explicit AllowFailedDetach call, by default they are protected from this these kind of risks, which indeed are very unlikely to be real issues within BCM realm.

[github-actions]

Metric	add-ace-to-pool-hook	develop
Coverage	69.8%	69.5%

[RensR]

Should you not check if it's already configured?

[RensR]

nit: not sure this is worth fuzz testing as no edge cases are tested, just input passing. Fuzz tests take >100 times longer to run

[RensR]

nit: cleaner as var instead of duplicating magic values