Conversation
| /** | ||
| * @title IPolicyEngine | ||
| * @dev Interface for the policy engine. | ||
| */ |
There was a problem hiding this comment.
This comment structure doesn't follow the style guide, but this is a completely internal project right?
There was a problem hiding this comment.
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
| @@ -0,0 +1,378 @@ | |||
| // SPDX-License-Identifier: BUSL-1.1 | |||
| pragma solidity ^0.8.20; | |||
There was a problem hiding this comment.
Unusual pragma for something with no imports
There was a problem hiding this comment.
This isn't resolved? What do you mean ditto?
| /// @param data The bytes to slice. | ||
| /// @param start The index starting from which to return the sub array. | ||
| /// @return Bytes sub array starting from start index. | ||
| function _slice( |
There was a problem hiding this comment.
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:]?
|
|
||
| Pool.LockOrBurnInV1 memory lockOrBurnIn = _createLockOrBurnIn(); | ||
|
|
||
| vm.stopPrank(); |
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| contract AdvancedPoolHooks_setPolicyEngineAllowFailedDetach is AdvancedPoolHooksSetup { |
There was a problem hiding this comment.
Why is this a different contract?
There was a problem hiding this comment.
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
| }); | ||
| bytes memory policyData = | ||
| abi.encodeWithSelector(CCIPPolicyEnginePayloads.POOL_HOOK_OUTBOUND_POLICY_DATA_V1_TAG, outboundData); | ||
| policyEngine.run( |
There was a problem hiding this comment.
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})
);
}
}
| poolAddr := common.HexToAddress(tpDeployReport.Output.Address) | ||
| hooksAddr := common.HexToAddress(hooksDeployReport.Output.Address) | ||
|
|
||
| applyAuthorizedCallerUpdatesReport, err := cldf_ops.ExecuteOperation(b, advanced_pool_hooks.ApplyAuthorizedCallerUpdates, chain, evm_contract.FunctionInput[advanced_pool_hooks.AuthorizedCallerArgs]{ |
There was a problem hiding this comment.
Should you not check if it's already configured?
|
|
||
| function testFuzz_postflightCheck_WithPolicyEngine( | ||
| bytes memory sourcePoolData, | ||
| bytes memory offchainTokenData |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
yeah questionable fuzzing value, changed to regular tests
|
|
||
| function test_postflightCheck_RevertWhen_PolicyEngineRejects() public { | ||
| s_advancedPoolHooks.setPolicyEngine(address(s_mockPolicyEngine)); | ||
| s_mockPolicyEngine.setShouldRevert(true, "Policy rejected"); |
There was a problem hiding this comment.
nit: cleaner as var instead of duplicating magic values
# Conflicts: # ccv/chains/evm/deployment/go.mod # ccv/chains/evm/deployment/go.sum # ccv/chains/evm/gobindings/generated/latest/advanced_pool_hooks/advanced_pool_hooks.go # chains/evm/.gas-snapshot # chains/evm/contracts/pools/AdvancedPoolHooks.sol # chains/evm/gobindings/generation/generated-wrapper-dependency-versions-do-not-edit.txt
|
5 participants
No description provided.