Skip to content

Conversation

@0xBasset
Copy link
Contributor

No description provided.

@0xBasset 0xBasset self-assigned this May 30, 2025
@0x3phemeralsoul 0x3phemeralsoul requested a review from oddaf June 4, 2025 13:39
0x3phemeralsoul
0x3phemeralsoul previously approved these changes Jul 7, 2025
oddaf
oddaf previously approved these changes Jul 17, 2025
@0xBasset 0xBasset dismissed stale reviews from oddaf and 0x3phemeralsoul via b336b8f September 17, 2025 13:13
Comment on lines 129 to 135
* [ ] Update safeharbor script
```bash
cd scripts/safeharbor
npm install
```
* [ ] Run `npm run generate` command in the `spells-mainnet` repo to check for bug bounty updates
* [ ] IF the command outputs hex encoded call:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't make all that available from the main Makefile?
Check the cast-on-tenderly entry there for reference.

* [ ] Run `npm run generate` command in the `spells-mainnet` repo to check for bug bounty updates
* [ ] IF the command outputs hex encoded call:
* [ ] Add ALL output call to the spell using low-level Solidity call.
* [ ] The call MUST use the pattern: `(bool succ, bytes memory err) = AGREEMENT.call(<encodedDATA>);`
Copy link
Contributor

@amusingaxl amusingaxl Sep 18, 2025

Choose a reason for hiding this comment

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

Aren't we using multicall now?
This way we can just pass the data and make a higher level call, which won't require us to check the success status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that the multicall accepts:

    struct Call {
        address target;
        bytes callData;
    }

function aggregate(Call[] memory calls);

Therefore, if there are multiple calls, we would need to use the annoying syntax of:

Call memory calls = new Call[](x);
calls[0] = { ... };
...
calls[x] = { ...};

multicall.aggregate(calls);

Which I personally think it's much worse in terms of clutter and amount of outputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, makes sense.
However there's another point, the script already provides the raw data for a low-level call to MULTICALL, so the checklist should reference this contract, not the agreement:

Suggested change
* [ ] The call MUST use the pattern: `(bool succ, bytes memory err) = AGREEMENT.call(<encodedDATA>);`
* [ ] The call MUST use the pattern: `(bool succ, bytes memory err) = MULTICALL.call(<encodedDATA>);`

* [ ] All actions are executed in the transaction trace
* [ ] No reverts are present that block execution
* [ ] No out-of-gas errors are present
* [ ] Confirm `make update-bug-bounty` returns empty
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [ ] Confirm `make update-bug-bounty` returns empty
* [ ] Confirm `make safeharbor-generate` returns empty

* [ ] End-to-end "happy path" interaction with the module
* IF bug bounty updates are present
* [ ] Test that all bug bounty registry calls execute successfully
* [ ] Verify `make update-bug-bounty` returns empty diff in test environment after spell execution
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [ ] Verify `make update-bug-bounty` returns empty diff in test environment after spell execution
* [ ] Verify `make safeharbor-generate` returns empty diff in test environment after spell execution

* [ ] End-to-end "happy path" interaction with the module
* IF bug bounty updates are present
* [ ] Test that all bug bounty registry calls execute successfully
* [ ] Verify `make update-bug-bounty` returns empty diff in test environment after spell execution
Copy link
Contributor

Choose a reason for hiding this comment

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

This specific step should probably be in the checklist right after "cast on tenderly".

* [ ] Target Contract is included in the ChainLog
* [ ] Test Coverage is comprehensive
* IF bug bounty registry updates are present
* [ ] Run `make safeharbor-verify calldata=0xhexEncodedData` command in the `spells-mainnet` repo, passing the calldata in the spell to check for it's validity.
Copy link
Contributor

@amusingaxl amusingaxl Sep 19, 2025

Choose a reason for hiding this comment

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

You don't need to explicitly mention the spells-mainnet repo, as it's implied. All other actions in the checklist happen there.

Suggested change
* [ ] Run `make safeharbor-verify calldata=0xhexEncodedData` command in the `spells-mainnet` repo, passing the calldata in the spell to check for it's validity.
* [ ] Run `make safeharbor-verify calldata=<encodedDATA>` command, passing the calldata in the spell to check for it's validity.

* [ ] Test Coverage is comprehensive
* IF bug bounty registry updates are present
* [ ] Run `make safeharbor-verify calldata=0xhexEncodedData` command in the `spells-mainnet` repo, passing the calldata in the spell to check for it's validity.
* [ ] Verify the call uses the correct pattern: `(bool succ, bytes memory err) = AGREEMENT.call(<encodedDATA>);`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [ ] Verify the call uses the correct pattern: `(bool succ, bytes memory err) = AGREEMENT.call(<encodedDATA>);`
* [ ] Verify the call uses the correct pattern: `(bool succ, bytes memory err) = MULTICALL.call(<encodedDATA>);`

* IF bug bounty registry updates are present
* [ ] Run `make safeharbor-verify calldata=<encodedDATA>` command passing the calldata in the spell to check for it's validity.
* [ ] Verify the call uses the correct pattern: `(bool succ, bytes memory err) = MULTICALL.call(<encodedDATA>);`
* [ ] Confirm proper error handling is implemented for each call
Copy link
Contributor

Choose a reason for hiding this comment

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

There will be only one call. Using "for each" here might be confusing.

Suggested change
* [ ] Confirm proper error handling is implemented for each call
* [ ] Confirm proper error handling is implemented for the call

* [ ] Run `make safeharbor-verify calldata=<encodedDATA>` command passing the calldata in the spell to check for it's validity.
* [ ] Verify the call uses the correct pattern: `(bool succ, bytes memory err) = MULTICALL.call(<encodedDATA>);`
* [ ] Confirm proper error handling is implemented for each call
* [ ] Verify the bug bounty section has appropriate comments/documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

"Appropriate" here is too subjective.
It's probably worth expanding which are the exact comments/documentation expectations.
Probably we should take the more detailed crafter check items and adapt them here.

Comment on lines 133 to 141
* [ ] If not already present, add the helper function to perform the call:
```solidity
function _updateSafeHarbor(bytes[] memory calldatas) public {
for (uint256 i = 0; i < calldatas.length; i++) {
(bool success, ) = address(AGREEMENT_ADDRESS).call(calldatas[i]);
require(success, "SaferHarbor call failed");
}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure we want to have the function in the checklist. Maybe we just reference the archive:

Suggested change
* [ ] If not already present, add the helper function to perform the call:
```solidity
function _updateSafeHarbor(bytes[] memory calldatas) public {
for (uint256 i = 0; i < calldatas.length; i++) {
(bool success, ) = address(AGREEMENT_ADDRESS).call(calldatas[i]);
require(success, "SaferHarbor call failed");
}
}
```
* [ ] If not already present, add the helper function to perform the call, using the established archive pattern.

amusingaxl
amusingaxl previously approved these changes Nov 15, 2025
amusingaxl
amusingaxl previously approved these changes Dec 4, 2025
Copy link
Contributor

@SidestreamBurningBanana SidestreamBurningBanana left a comment

Choose a reason for hiding this comment

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

Approving with 2 nits present, will give re-approval if they are resolved

@0xBasset 0xBasset merged commit 1ac5b59 into master Dec 18, 2025
2 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.

6 participants