Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rc-v1.0.1-stable proposed bytecode optimization to merge into stable branch #91

Merged
merged 12 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .gas-snapshot
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
CirclesTest:testCalculateIssuance() (gas: 3729002)
CirclesTest:testConsecutiveClaimablePeriods() (gas: 375082)
CirclesTest:testDemurragedTransfer() (gas: 229055)
CompositeMintGroupsTest:testCompositeGroupMint() (gas: 212459)
DemurrageTest:testDemurrageFactor() (gas: 79139)
DemurrageTest:testFuzzStablePointIssuance(int192) (runs: 256, μ: 63114, ~: 63250)
DemurrageTest:testInversionGammaBeta64x64_100years() (gas: 958541)
DemurrageTest:testInversionGammaBeta64x64_100years_withExtension() (gas: 1083080)
DemurrageTest:testInversionGammaBeta64x64_100years_withExtension_comparison() (gas: 1703713)
DemurrageTest:testInversionGammaBeta64x64_20years() (gas: 221562)
DemurrageTest:testRepeatedDemurrage() (gas: 15264702)
ERC20LiftTest:testERC20Demurrage() (gas: 133270)
ERC20LiftTest:testERC20Wrap() (gas: 954629)
ERC20LiftTest:testWrapAndUnwrapInflationaryERC20() (gas: 89881)
GroupMintTest:testRegisterGroup() (gas: 165153)
HubPathTransferTest:testOperateFlowMatrixConsentedFlow() (gas: 265369)
MigrationTest:testConversionMigrationV1ToTimeCircles() (gas: 18365)
MintGroupCirclesTest:testDirectSelfGroupMintFails() (gas: 341135)
MintGroupCirclesTest:testGroupMint() (gas: 340165)
MintGroupCirclesTest:testGroupMintFail() (gas: 23302)
MintGroupCirclesTest:testGroupMintMany() (gas: 858065)
MintGroupCirclesTest:testGroupMintMultiCollateral() (gas: 755126)
MintGroupCirclesTest:testSequentialGroupMint() (gas: 511760)
NamesTest:testBase58Conversion() (gas: 78904)
NamesTest:testCustomName() (gas: 40809)
NamesTest:testInvalidCustomNames() (gas: 20017)
NamesTest:testMetadataDigest() (gas: 35073)
NamesTest:testShortName() (gas: 102230)
NamesTest:testShortNameWithNonce() (gas: 73360)
NamesTest:testShortNameWithPadding() (gas: 71662)
V1MintStatusUpdateTest:testMigrationFromV1DuringBootstrap() (gas: 2542748)
14 changes: 12 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ name: test

on:
push:
branches: [ master, develop ]
branches: [ master, develop, beta, candidate/stable ]
pull_request:
branches: [ master, develop ]
branches: [ master, develop, beta, candidate/stable ]
workflow_dispatch:

env:
Expand Down Expand Up @@ -42,3 +42,13 @@ jobs:
run: |
forge test -vvv
id: test

- name: Print compiled non-test contract sizes
run: |
forge build --skip test --sizes
id: build-sizes

- name: Print gas difference against a pre-existing snapshot
run: |
forge snapshot --diff
id: gas-diff
2 changes: 0 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ out/
# docs


# gas snapshot files
.gas-snapshot

# Ignores development broadcast logs
!/broadcast
Expand Down
5 changes: 1 addition & 4 deletions src/circles/DiscountedBalances.sol
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,7 @@ contract DiscountedBalances is Demurrage {
// revert CirclesDemurrageAmountExceedsMaxUint192(_account, _id, _balance, 0);
revert CirclesErrorAddressUintArgs(_account, _id, 0x81);
}
DiscountedBalance memory discountedBalance = discountedBalances[_id][_account];
Copy link
Member

Choose a reason for hiding this comment

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

this is possibly an improvement, but as mentioned some of these changes were done on recommendation of reviewers. so my general comment holds, i would like to start measuring explicitly to see what is an optimisation in numbers

discountedBalance.balance = uint192(_balance);
discountedBalance.lastUpdatedDay = _day;
discountedBalances[_id][_account] = discountedBalance;
discountedBalances[_id][_account] = DiscountedBalance({balance: uint192(_balance), lastUpdatedDay: _day});
}

/**
Expand Down
20 changes: 12 additions & 8 deletions src/circles/ERC1155.sol
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,7 @@ abstract contract ERC1155 is DiscountedBalances, Context, ERC165, IERC1155, IERC
* @dev See {IERC1155-safeTransferFrom}.
*/
function safeTransferFrom(address _from, address _to, uint256 _id, uint256 _value, bytes memory _data) public {
address sender = _msgSender();
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned before, I tried to not update this forked contract from open zepellin unless where functionally needed. that said, sure this seems like a fair readability improvement.

It would be good to have measurements in place in CI so we can see whether this increases or reduces different metrics. I don't want to walk blind for optimisations

if (_from != sender && !isApprovedForAll(_from, sender)) {
revert ERC1155MissingApprovalForAll(sender, _from);
}
_allowanceCheck(_from);
_safeTransferFrom(_from, _to, _id, _value, _data);
}

Expand All @@ -131,10 +128,7 @@ abstract contract ERC1155 is DiscountedBalances, Context, ERC165, IERC1155, IERC
uint256[] memory _values,
bytes memory _data
) public virtual {
address sender = _msgSender();
if (_from != sender && !isApprovedForAll(_from, sender)) {
revert ERC1155MissingApprovalForAll(sender, _from);
}
_allowanceCheck(_from);
_safeBatchTransferFrom(_from, _to, _ids, _values, _data);
}

Expand Down Expand Up @@ -216,6 +210,16 @@ abstract contract ERC1155 is DiscountedBalances, Context, ERC165, IERC1155, IERC
_acceptanceCheck(from, to, ids, values, data);
}

/**
* @dev do the ERC1155 token the sender allowance check
*/
function _allowanceCheck(address _from) internal view {
address sender = _msgSender();
if (_from != sender && !isApprovedForAll(_from, sender)) {
revert ERC1155MissingApprovalForAll(sender, _from);
}
}

/**
* @dev do the ERC1155 token acceptance check to the receiver
*/
Expand Down
39 changes: 23 additions & 16 deletions src/hub/Hub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ contract Hub is Circles, TypeDefinitions, IHubErrors {

// store the metadata digest for the avatar metadata
if (_metadataDigest != bytes32(0)) {
nameRegistry.setMetadataDigest(msg.sender, _metadataDigest);
_setMetadataDigest(_metadataDigest);
}
}

Expand All @@ -287,16 +287,7 @@ contract Hub is Circles, TypeDefinitions, IHubErrors {
function registerGroup(address _mint, string calldata _name, string calldata _symbol, bytes32 _metadataDigest)
external
{
_registerGroup(msg.sender, _mint, standardTreasury, _name, _symbol);

// for groups register possible custom name and symbol
nameRegistry.registerCustomName(msg.sender, _name);
nameRegistry.registerCustomSymbol(msg.sender, _symbol);

// store the IPFS CIDv0 digest for the group metadata
nameRegistry.setMetadataDigest(msg.sender, _metadataDigest);

emit RegisterGroup(msg.sender, _mint, standardTreasury, _name, _symbol);
registerCustomGroup(_mint, standardTreasury, _name, _symbol, _metadataDigest);
}

/**
Expand All @@ -313,15 +304,15 @@ contract Hub is Circles, TypeDefinitions, IHubErrors {
string calldata _name,
string calldata _symbol,
bytes32 _metadataDigest
) external {
) public {
_registerGroup(msg.sender, _mint, _treasury, _name, _symbol);

// for groups register possible custom name and symbol
nameRegistry.registerCustomName(msg.sender, _name);
_registerCustomName(_name);
nameRegistry.registerCustomSymbol(msg.sender, _symbol);

// store the metadata digest for the group metadata
nameRegistry.setMetadataDigest(msg.sender, _metadataDigest);
_setMetadataDigest(_metadataDigest);

emit RegisterGroup(msg.sender, _mint, _treasury, _name, _symbol);
}
Expand All @@ -335,10 +326,10 @@ contract Hub is Circles, TypeDefinitions, IHubErrors {
_insertAvatar(msg.sender);

// for organizations, only register possible custom name
nameRegistry.registerCustomName(msg.sender, _name);
_registerCustomName(_name);

// store the IPFS CIDv0 digest for the organization metadata
nameRegistry.setMetadataDigest(msg.sender, _metadataDigest);
_setMetadataDigest(_metadataDigest);

emit RegisterOrganization(msg.sender, _name);
}
Expand Down Expand Up @@ -1228,4 +1219,20 @@ contract Hub is Circles, TypeDefinitions, IHubErrors {
// update the expiry; checks must be done by caller
trustMarker.expiry = _expiry;
}

/**
* @dev Calls nameRegistry to store the metadata digest.
* @param _metadataDigest sha256 metadata digest for the avatar metadata.
*/
function _setMetadataDigest(bytes32 _metadataDigest) private {
nameRegistry.setMetadataDigest(msg.sender, _metadataDigest);
}

/**
* @dev Calls nameRegistry to register the custom name.
* @param _name immutable name of the group Circles or organization.
*/
function _registerCustomName(string calldata _name) private {
nameRegistry.registerCustomName(msg.sender, _name);
}
}
1 change: 1 addition & 0 deletions test/hub/MockDeployment.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import "../lift/MockERC20Lift.sol";
import "./MockHub.sol";

contract MockDeployment {
bool public IS_TEST = true;
// State variables

MockHub public hub;
Expand Down
1 change: 1 addition & 0 deletions test/hub/MockHub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity >=0.8.24;
import "../../src/hub/Hub.sol";

contract MockHub is Hub {
bool public IS_TEST = true;
// Constructor

constructor(uint256 _inflationDayZero, uint256 _bootstrapTime)
Expand Down
1 change: 1 addition & 0 deletions test/hub/MockPathTransferHub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "../../src/migration/IHub.sol";
import "../../src/names/INameRegistry.sol";

contract MockPathTransferHub is Hub {
bool public IS_TEST = true;
// Constructor

constructor(uint256 _inflationDayZero, uint256 _bootstrapTime)
Expand Down
Loading