Skip to content

Commit

Permalink
Handle pipefail in bash contract scripts (#488)
Browse files Browse the repository at this point in the history
The `set -e` set in the main script swallows all errors and custom error
handling does not really work.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Chores**
- Enhanced error handling and logging in deployment scripts, Docker
builds, and CI workflows to reduce silent failures.
- **Refactor**
- Standardized naming in contract interfaces for improved readability
and consistency.
- **Tests**
- Updated test naming and revert expectations to clearly signal error
conditions during failure scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Borja Aranda <[email protected]>
  • Loading branch information
mkysel and fbac authored Feb 17, 2025
1 parent e5665dc commit 2cc4b91
Show file tree
Hide file tree
Showing 11 changed files with 297 additions and 119 deletions.
3 changes: 1 addition & 2 deletions .github/workflows/build-pre-baked-images.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,4 @@ jobs:
file: ./dev/baked/Dockerfile
push: ${{ github.event_name != 'pull_request' }}
tags: ${{ steps.meta.outputs.tags }}
labels: ${{ steps.meta.outputs.labels }}
build-args: "FOUNDRY_VERSION=nightly-ac81a53d1d5823919ffbadd3c65f081927aa11f2"
labels: ${{ steps.meta.outputs.labels }}
2 changes: 2 additions & 0 deletions .github/workflows/solidity.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ jobs:

- name: Install Foundry
uses: foundry-rs/foundry-toolchain@v1
with:
version: v0.3.0

- name: Set cache key
id: set-cache-key
Expand Down
2 changes: 1 addition & 1 deletion contracts/dev/generate
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ function generate_bindings() {
--bin "${build_artifact}.bin.json" \
--pkg "${package}" \
--type "${filename}" \
--out "${output_artifact}" > /dev/null 2>&1; then
--out "${output_artifact}"; then
echo "ERROR: Failed to generate bindings for ${filename}" >&2
exit 1
fi
Expand Down
25 changes: 12 additions & 13 deletions contracts/dev/lib/common
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#!/bin/bash
set -euo pipefail

function get_chain_id() {
hex_chain_id=$(curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_chainId","id":1}' ${RPC_URL} | jq -r '.result')
Expand All @@ -11,8 +10,8 @@ function forge_deploy_script() {
case $1 in
group_messages)
echo "⧖ Deploying GroupMessages to chainId ${chain_id} using RPC ${RPC_URL}"
forge script --quiet --rpc-url "${RPC_URL}" --broadcast script/DeployGroupMessages.s.sol
if [ $? -ne 0 ]; then
forge script --quiet --rpc-url "${RPC_URL}" --broadcast script/DeployGroupMessages.s.sol || BUILD_FAILED=true
if [[ -n "${BUILD_FAILED:-}" ]]; then
echo "Failed to deploy group messages contract"
exit 1
fi
Expand All @@ -22,8 +21,8 @@ function forge_deploy_script() {

identity_updates)
echo "⧖ Deploying IdentityUpdates to chainId ${chain_id} using RPC ${RPC_URL}"
forge script --quiet --rpc-url "${RPC_URL}" --broadcast script/DeployIdentityUpdates.s.sol
if [ $? -ne 0 ]; then
forge script --quiet --rpc-url "${RPC_URL}" --broadcast script/DeployIdentityUpdates.s.sol || BUILD_FAILED=true
if [[ -n "${BUILD_FAILED:-}" ]]; then
echo "Failed to deploy identity updates contract"
exit 1
fi
Expand All @@ -48,8 +47,8 @@ function forge_deploy_script() {
function forge_clean() {
echo -e "⧖ Cleaning old artifacts"

forge clean &> .forge_clean.tmp.log
if [ $? -ne 0 ]; then
forge clean &> .forge_clean.tmp.log || BUILD_FAILED=true
if [[ -n "${BUILD_FAILED:-}" ]]; then
echo "ERROR: Failed to clean old artifacts"
cat .forge_clean.tmp.log
exit 1
Expand All @@ -62,8 +61,8 @@ function forge_clean() {
function forge_soldeer_update() {
echo -e "⧖ Updating dependencies"

forge soldeer update &> .forge_soldeer_update.tmp.log
if [ $? -ne 0 ]; then
forge soldeer update &> .forge_soldeer_update.tmp.log || BUILD_FAILED=true
if [[ -n "${BUILD_FAILED:-}" ]]; then
echo "ERROR: Failed to update dependencies"
cat .forge_soldeer_update.tmp.log
exit 1
Expand All @@ -76,8 +75,8 @@ function forge_soldeer_update() {
function forge_build_contracts() {
echo -e "⧖ Building contracts"

forge build &> .forge_build.tmp.log
if [ $? -ne 0 ]; then
forge build &> .forge_build.tmp.log || BUILD_FAILED=true
if [[ -n "${BUILD_FAILED:-}" ]]; then
echo "ERROR: Failed to build contracts"
cat .forge_build.tmp.log
exit 1
Expand All @@ -90,8 +89,8 @@ function forge_build_contracts() {
function forge_test_contracts() {
echo -e "⧖ Running contract tests"

forge test &> .forge_test.tmp.log
if [ $? -ne 0 ]; then
forge test &> .forge_test.tmp.log || BUILD_FAILED=true
if [[ -n "${BUILD_FAILED:-}" ]]; then
echo "ERROR: Tests failed"
cat .forge_test.tmp.log
exit 1
Expand Down
177 changes: 177 additions & 0 deletions contracts/output.sarif
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
{
"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
"version": "2.1.0",
"runs": [
{
"tool": {
"driver": {
"name": "Slither",
"informationUri": "https://github.com/crytic/slither",
"version": "0.11.0",
"rules": [
{
"id": "3-0-naming-convention",
"name": "naming-convention",
"properties": {
"precision": "very-high",
"security-severity": "0.0"
},
"shortDescription": {
"text": "Conformance to Solidity naming conventions"
},
"help": {
"text": "Follow the Solidity [naming convention](https://solidity.readthedocs.io/en/v0.4.25/style-guide.html#naming-conventions)."
}
}
]
}
},
"results": [
{
"ruleId": "3-0-naming-convention",
"message": {
"text": "Parameter GroupMessages.setMaxPayloadSize(uint256)._maxPayloadSize (src/GroupMessages.sol#116) is not in mixedCase\n",
"markdown": "Parameter [GroupMessages.setMaxPayloadSize(uint256)._maxPayloadSize](src/GroupMessages.sol#L116) is not in mixedCase\n"
},
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/GroupMessages.sol"
},
"region": {
"startLine": 116,
"endLine": 116
}
}
}
],
"partialFingerprints": {
"id": "2ac15f70bb46de53947ad37fb235f6282fabbf46a800cae89e2ac8785bb88c3a"
}
},
{
"ruleId": "3-0-naming-convention",
"message": {
"text": "Parameter IdentityUpdates.setMaxPayloadSize(uint256)._maxPayloadSize (src/IdentityUpdates.sol#115) is not in mixedCase\n",
"markdown": "Parameter [IdentityUpdates.setMaxPayloadSize(uint256)._maxPayloadSize](src/IdentityUpdates.sol#L115) is not in mixedCase\n"
},
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/IdentityUpdates.sol"
},
"region": {
"startLine": 115,
"endLine": 115
}
}
}
],
"partialFingerprints": {
"id": "62490d724a19f5e3423e051e95d10e2b113ef51064439df4cf44fd8f811183c6"
}
},
{
"ruleId": "3-0-naming-convention",
"message": {
"text": "Parameter GroupMessages.setMinPayloadSize(uint256)._minPayloadSize (src/GroupMessages.sol#105) is not in mixedCase\n",
"markdown": "Parameter [GroupMessages.setMinPayloadSize(uint256)._minPayloadSize](src/GroupMessages.sol#L105) is not in mixedCase\n"
},
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/GroupMessages.sol"
},
"region": {
"startLine": 105,
"endLine": 105
}
}
}
],
"partialFingerprints": {
"id": "72c177d760e4c601e6f19288c559517eda0552df7640e9c8b7add27163a27bef"
}
},
{
"ruleId": "3-0-naming-convention",
"message": {
"text": "Parameter IdentityUpdates.setMinPayloadSize(uint256)._minPayloadSize (src/IdentityUpdates.sol#104) is not in mixedCase\n",
"markdown": "Parameter [IdentityUpdates.setMinPayloadSize(uint256)._minPayloadSize](src/IdentityUpdates.sol#L104) is not in mixedCase\n"
},
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/IdentityUpdates.sol"
},
"region": {
"startLine": 104,
"endLine": 104
}
}
}
],
"partialFingerprints": {
"id": "99c1e6ae63e16538953a6c34d5c41b0a42f9dc77cd8de57f29584fe9dd03440b"
}
},
{
"ruleId": "3-0-naming-convention",
"message": {
"text": "Parameter IdentityUpdates.initialize(address)._admin (src/IdentityUpdates.sol#57) is not in mixedCase\n",
"markdown": "Parameter [IdentityUpdates.initialize(address)._admin](src/IdentityUpdates.sol#L57) is not in mixedCase\n"
},
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/IdentityUpdates.sol"
},
"region": {
"startLine": 57,
"endLine": 57
}
}
}
],
"partialFingerprints": {
"id": "b5a63ece800ba46149c579a641aaf4bd23358305eb6618b9a77cfcc16813fc23"
}
},
{
"ruleId": "3-0-naming-convention",
"message": {
"text": "Parameter GroupMessages.initialize(address)._admin (src/GroupMessages.sol#57) is not in mixedCase\n",
"markdown": "Parameter [GroupMessages.initialize(address)._admin](src/GroupMessages.sol#L57) is not in mixedCase\n"
},
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "src/GroupMessages.sol"
},
"region": {
"startLine": 57,
"endLine": 57
}
}
}
],
"partialFingerprints": {
"id": "c5b032b7ea60099295cbecbf5bb84c139f75fd4e8fcbb541f5009b07b6ae3a8a"
}
}
]
}
]
}
58 changes: 29 additions & 29 deletions contracts/pkg/groupmessages/GroupMessages.go

Large diffs are not rendered by default.

58 changes: 29 additions & 29 deletions contracts/pkg/identityupdates/IdentityUpdates.go

Large diffs are not rendered by default.

32 changes: 16 additions & 16 deletions contracts/src/GroupMessages.sol
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ contract GroupMessages is Initializable, AccessControlUpgradeable, UUPSUpgradeab

// Initialization
/// @notice Initializes the contract with the deployer as admin.
/// @param _admin The address of the admin.
function initialize(address _admin) public initializer {
require(_admin != address(0), ZeroAdminAddress());
/// @param admin The address of the admin.
function initialize(address admin) public initializer {
require(admin != address(0), ZeroAdminAddress());

__UUPSUpgradeable_init();
__AccessControl_init();
Expand All @@ -64,7 +64,7 @@ contract GroupMessages is Initializable, AccessControlUpgradeable, UUPSUpgradeab
minPayloadSize = 78;
maxPayloadSize = 4_194_304;

_grantRole(DEFAULT_ADMIN_ROLE, _admin);
_grantRole(DEFAULT_ADMIN_ROLE, admin);
}

// Pausable functionality
Expand Down Expand Up @@ -100,25 +100,25 @@ contract GroupMessages is Initializable, AccessControlUpgradeable, UUPSUpgradeab
}

/// @notice Sets the minimum payload size
/// @param _minPayloadSize The new minimum payload size
/// @param minPayloadSizeRequest The new minimum payload size
/// @dev Ensures the new minimum is less than the maximum
function setMinPayloadSize(uint256 _minPayloadSize) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(_minPayloadSize < maxPayloadSize, InvalidMinPayloadSize());
require(_minPayloadSize > 0, InvalidMinPayloadSize());
function setMinPayloadSize(uint256 minPayloadSizeRequest) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(minPayloadSizeRequest < maxPayloadSize, InvalidMinPayloadSize());
require(minPayloadSizeRequest > 0, InvalidMinPayloadSize());
uint256 oldSize = minPayloadSize;
minPayloadSize = _minPayloadSize;
emit MinPayloadSizeUpdated(oldSize, _minPayloadSize);
minPayloadSize = minPayloadSizeRequest;
emit MinPayloadSizeUpdated(oldSize, minPayloadSize);
}

/// @notice Sets the maximum payload size
/// @param _maxPayloadSize The new maximum payload size
/// @param maxPayloadSizeRequest The new maximum payload size
/// @dev Ensures the new maximum is greater than the minimum
function setMaxPayloadSize(uint256 _maxPayloadSize) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(_maxPayloadSize > minPayloadSize, InvalidMaxPayloadSize());
require(_maxPayloadSize <= 4_194_304, InvalidMaxPayloadSize());
function setMaxPayloadSize(uint256 maxPayloadSizeRequest) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(maxPayloadSizeRequest > minPayloadSize, InvalidMaxPayloadSize());
require(maxPayloadSizeRequest <= 4_194_304, InvalidMaxPayloadSize());
uint256 oldSize = maxPayloadSize;
maxPayloadSize = _maxPayloadSize;
emit MaxPayloadSizeUpdated(oldSize, _maxPayloadSize);
maxPayloadSize = maxPayloadSizeRequest;
emit MaxPayloadSizeUpdated(oldSize, maxPayloadSize);
}

// Upgradeability
Expand Down
32 changes: 16 additions & 16 deletions contracts/src/IdentityUpdates.sol
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ contract IdentityUpdates is Initializable, AccessControlUpgradeable, UUPSUpgrade

// Initialization
/// @notice Initializes the contract with the deployer as admin.
/// @param _admin The address of the admin.
function initialize(address _admin) public initializer {
require(_admin != address(0), ZeroAdminAddress());
/// @param admin The address of the admin.
function initialize(address admin) public initializer {
require(admin != address(0), ZeroAdminAddress());

__UUPSUpgradeable_init();
__AccessControl_init();
Expand All @@ -64,7 +64,7 @@ contract IdentityUpdates is Initializable, AccessControlUpgradeable, UUPSUpgrade
minPayloadSize = 104;
maxPayloadSize = 4_194_304;

_grantRole(DEFAULT_ADMIN_ROLE, _admin);
_grantRole(DEFAULT_ADMIN_ROLE, admin);
}

// Pausable functionality
Expand Down Expand Up @@ -99,25 +99,25 @@ contract IdentityUpdates is Initializable, AccessControlUpgradeable, UUPSUpgrade
}

/// @notice Sets the minimum payload size
/// @param _minPayloadSize The new minimum payload size
/// @param minPayloadSizeRequest The new minimum payload size
/// @dev Ensures the new minimum is less than the maximum
function setMinPayloadSize(uint256 _minPayloadSize) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(_minPayloadSize < maxPayloadSize, InvalidMinPayloadSize());
require(_minPayloadSize > 0, InvalidMinPayloadSize());
function setMinPayloadSize(uint256 minPayloadSizeRequest) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(minPayloadSizeRequest < maxPayloadSize, InvalidMinPayloadSize());
require(minPayloadSizeRequest > 0, InvalidMinPayloadSize());
uint256 oldSize = minPayloadSize;
minPayloadSize = _minPayloadSize;
emit MinPayloadSizeUpdated(oldSize, _minPayloadSize);
minPayloadSize = minPayloadSizeRequest;
emit MinPayloadSizeUpdated(oldSize, minPayloadSizeRequest);
}

/// @notice Sets the maximum payload size
/// @param _maxPayloadSize The new maximum payload size
/// @param maxPayloadSizeRequest The new maximum payload size
/// @dev Ensures the new maximum is greater than the minimum
function setMaxPayloadSize(uint256 _maxPayloadSize) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(_maxPayloadSize > minPayloadSize, InvalidMaxPayloadSize());
require(_maxPayloadSize <= 4_194_304, InvalidMaxPayloadSize());
function setMaxPayloadSize(uint256 maxPayloadSizeRequest) public onlyRole(DEFAULT_ADMIN_ROLE) {
require(maxPayloadSizeRequest > minPayloadSize, InvalidMaxPayloadSize());
require(maxPayloadSizeRequest <= 4_194_304, InvalidMaxPayloadSize());
uint256 oldSize = maxPayloadSize;
maxPayloadSize = _maxPayloadSize;
emit MaxPayloadSizeUpdated(oldSize, _maxPayloadSize);
maxPayloadSize = maxPayloadSizeRequest;
emit MaxPayloadSizeUpdated(oldSize, maxPayloadSizeRequest);
}

// Upgradeability
Expand Down
Loading

0 comments on commit 2cc4b91

Please sign in to comment.