Skip to content

Commit

Permalink
Remove the need for --via-ir in our contracts (#502)
Browse files Browse the repository at this point in the history
### Description

After getting more and more annoyed with the CI time and other small
issues around this I decided to really understand how big is our need
for `--via-ir` compilation. Turns out it's really small. I mean really
small.

There are just a couple of places where this is causing issues: 

1. `Airgrab#claim` has too many variables. I fixed this with a `struct`
for the fractal proof, and an internal function to do the lock. Changing
it is ok in my opinion. If we want to reuse this contract we'll have to
redeploy it anyway, we have a tag for the version when it was deployed,
all is well.

2. `GovernanceFactory#createGovernance` has too many variables. I just
broke it down in internal functions. Again, I don't see a problem in
changing this, even if it's deployed and etc.

3. `StableTokenV2#initialize` has too many arguments. When we made
StableTokenV2 we wanted to keep the same interface for the `initialize`
method to keep the interfaces between stable tokens consistent, but it
was actually a bad call. There's no good reason why we have it that way
and it's a method that can anyway only be called once. I would say it's
ok to also change this without making a StableTokenV3 for it. Basically
we will have a few contracts which will have a different signature for
the initialize method that anyway can't be called anymore. But this is
the one change the feels least nice.

4. A couple of tests with many variables where I used structs for
grouping or broke down into internal functions.

What happens afterwards, well:
```
╰─ forge test
[⠊] Compiling...
[⠒] Compiling 64 files with Solc 0.5.17
[⠢] Compiling 55 files with Solc 0.8.26
[⠒] Compiling 223 files with Solc 0.8.18
[⠒] Solc 0.5.17 finished in 307.90ms
[⠘] Solc 0.8.26 finished in 1.75s
[⠒] Solc 0.8.18 finished in 5.50s
Compiler run successful with warnings:
```
The whole fucking shabang runs in 5.5s. Jesus.
CI takes ~1minute instead of ~15minutes.

P.S. Thank you Claude for the bash script.

### Other changes

It changed my life.

### Tested

Thoroughly.

### Related issues

Sleep deprivation.

### Backwards compatibility

Who are you calling backward?

### Documentation

I read it, yes.

---------

Co-authored-by: Bayological <6872903+bayological@users.noreply.github.com>
Co-authored-by: chapati <philip.paetz@me.com>
Co-authored-by: philbow61 <80156619+philbow61@users.noreply.github.com>
Co-authored-by: baroooo <baranseltekin@gmail.com>
  • Loading branch information
5 people authored Aug 27, 2024
1 parent 45a551d commit d916e50
Show file tree
Hide file tree
Showing 16 changed files with 386 additions and 255 deletions.
6 changes: 2 additions & 4 deletions .github/workflows/lint_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,8 @@ jobs:
- name: "Run the tests"
run: "forge test"

- name: "Build the contracts"
run: |
forge --version
forge build --sizes --skip test/**/*
- name: "Check contract sizes"
run: "yarn run check-contract-sizes"

- name: "Add test summary"
run: |
Expand Down
11 changes: 10 additions & 1 deletion .prettierrc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,21 @@ singleQuote: false
tabWidth: 2
trailingComma: all

plugins: ["prettier-plugin-solidity"]
plugins: [prettier-plugin-solidity]

overrides:
- files: ["*.sol"]
options:
compiler: 0.5.17
- files: [contracts/interfaces/*.sol]
options:
compiler: 0.8.18
- files:
- contracts/interfaces/IBrokerAdmin.sol
- contracts/interfaces/ICeloToken.sol
- contracts/interfaces/IExchange.sol
options:
compiler: 0.5.17
- files: [contracts/tokens/patched/*.sol]
options:
compiler: 0.8.18
Expand Down
65 changes: 65 additions & 0 deletions bin/check-contracts.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#!/bin/bash
##############################################################################
# Sometimes when hitting AST compiler errors, the output doesn't tell you
# what file is causing the issue. This script helps you identify which
# contract is failing by compiling each contract individually.
##############################################################################

# Get all contract files
IFS=$'\n' read -r -d '' -a contract_files < <(find contracts test -name "*.sol" && printf '\0')

# Initialize an array to store contracts that need --via-ir
via_ir_contracts=()

# Function to check if a contract builds without --via-ir
check_contract() {
local target_contract=$1
local skip_contracts=()

for contract in "${contract_files[@]}"; do
if [ "$contract" != "$target_contract" ]; then
skip_contracts+=("$contract")
fi
done

forge clean
if forge build --skip ${skip_contracts[*]}; then
return 0
else
return 1
fi
}

# Iterate through each contract
for contract in "${contract_files[@]}"; do
echo "----------------------------------------"
echo "Checking $contract..."
if check_contract "$contract"; then
echo "$contract does not require --via-ir"
else
echo "$contract requires --via-ir"
via_ir_contracts+=("$contract")
fi
echo "----------------------------------------"
echo
done

# Print the results
if [ ${#via_ir_contracts[@]} -eq 0 ]; then
echo "All contracts can be built without --via-ir."
else
echo "The following contracts require --via-ir:"
printf '%s\n' "${via_ir_contracts[@]}"
echo
echo "Use the following command to build:"
echo -n "forge build --via-ir --skip "

contracts_to_skip=()
for contract in "${contract_files[@]}"; do
if [[ ! " ${via_ir_contracts[*]} " =~ " ${contract} " ]]; then
contracts_to_skip+=("$contract")
fi
done

echo "${contracts_to_skip[*]} test/**/*"
fi
62 changes: 32 additions & 30 deletions contracts/governance/Airgrab.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@ contract Airgrab is ReentrancyGuard {
uint32 public constant MAX_CLIFF_PERIOD = 103;
uint32 public constant MAX_SLOPE_PERIOD = 104;

/**
* @notice FractalProof struct for KYC/KYB verification.
* @param sig The signature of the Fractal Credential.
* @param validUntil The timestamp when the Fractal Credential expires.
* @param approvedAt The timestamp when the Fractal Credential was approved.
* @param fractalId The Fractal Credential ID.
*/
struct FractalProof {
bytes sig;
uint256 validUntil;
uint256 approvedAt;
string fractalId;
}

/**
* @notice Emitted when tokens are claimed
* @param claimer The account claiming the tokens
Expand Down Expand Up @@ -68,39 +82,30 @@ contract Airgrab is ReentrancyGuard {
* https://github.com/trustfractal/credentials-api-verifiers
* @notice This function checks the kyc signature with the data provided.
* @param account The address of the account to check.
* @param proof The kyc proof for the account.
* @param validUntil The kyc proof valid until timestamp.
* @param approvedAt The kyc proof approved at timestamp.
* @param fractalId The kyc proof fractal id.
* @param proof FractaLProof kyc proof data for the account.
*/
modifier hasValidKyc(
address account,
bytes memory proof,
uint256 validUntil,
uint256 approvedAt,
string memory fractalId
) {
require(block.timestamp < validUntil, "Airgrab: KYC no longer valid");
require(fractalMaxAge == 0 || block.timestamp < approvedAt + fractalMaxAge, "Airgrab: KYC not recent enough");
modifier hasValidKyc(address account, FractalProof memory proof) {
require(block.timestamp < proof.validUntil, "Airgrab: KYC no longer valid");
require(fractalMaxAge == 0 || block.timestamp < proof.approvedAt + fractalMaxAge, "Airgrab: KYC not recent enough");
string memory accountString = Strings.toHexString(uint256(uint160(account)), 20);

bytes32 signedMessageHash = ECDSA.toEthSignedMessageHash(
abi.encodePacked(
accountString,
";",
fractalId,
proof.fractalId,
";",
Strings.toString(approvedAt),
Strings.toString(proof.approvedAt),
";",
Strings.toString(validUntil),
Strings.toString(proof.validUntil),
";",
// ISO 3166-1 alpha-2 country codes
// DRC, CUBA, GB, IRAN, DPKR, MALI, MYANMAR, SOUTH SUDAN, SYRIA, US, YEMEN
"level:plus+liveness;citizenship_not:;residency_not:cd,cu,gb,ir,kp,ml,mm,ss,sy,us,ye"
)
);

require(SignatureChecker.isValidSignatureNow(fractalSigner, signedMessageHash, proof), "Airgrab: Invalid KYC");
require(SignatureChecker.isValidSignatureNow(fractalSigner, signedMessageHash, proof.sig), "Airgrab: Invalid KYC");

_;
}
Expand Down Expand Up @@ -178,26 +183,23 @@ contract Airgrab is ReentrancyGuard {
* @param delegate The address of the account that gets voting power delegated
* @param merkleProof The merkle proof for the account.
* @param fractalProof The Fractal KYC proof for the account.
* @param fractalProofValidUntil The Fractal KYC proof valid until timestamp.
* @param fractalProofApprovedAt The Fractal KYC proof approved at timestamp.
* @param fractalId The Fractal KYC ID.
*/
function claim(
uint96 amount,
address delegate,
bytes32[] calldata merkleProof,
bytes calldata fractalProof,
uint256 fractalProofValidUntil,
uint256 fractalProofApprovedAt,
string memory fractalId
)
external
hasValidKyc(msg.sender, fractalProof, fractalProofValidUntil, fractalProofApprovedAt, fractalId)
canClaim(msg.sender, amount, merkleProof)
nonReentrant
{
FractalProof calldata fractalProof
) external hasValidKyc(msg.sender, fractalProof) canClaim(msg.sender, amount, merkleProof) nonReentrant {
require(token.balanceOf(address(this)) >= amount, "Airgrab: insufficient balance");
_claim(amount, delegate);
}

/**
* @dev Internal function to claim tokens and lock them.
* @param amount The amount of tokens to be claimed.
* @param delegate The address of the account that gets voting power delegated
*/
function _claim(uint96 amount, address delegate) internal {
claimed[msg.sender] = true;
uint256 lockId = locking.lock(msg.sender, delegate, amount, slopePeriod, cliffPeriod);
emit TokensClaimed(msg.sender, amount, lockId);
Expand Down
Loading

0 comments on commit d916e50

Please sign in to comment.