Skip to content
Open
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
42 changes: 21 additions & 21 deletions l1-contracts/GUIDE_LINES.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
# Smart Contract Guideline
In the following, there will be guidelines for the process of writing readable, maintainable and secure contract code. We try to cover the entire process:
- before writing the code,
- while writing,
In the following, there will be guidelines for the process of writing readable, maintainable and secure contract code. We try to cover the entire process:
- before writing the code,
- while writing,
- when optimizing
- when reviewing.
- when reviewing.

In general the language of choice will be Solidity for our L1 contracts, so most of the specific style rules will be having that in mind, but the process for writing it can be adapted slightly for vyper, and will have an extension when Aztec.nr contracts are more mature.
In general the language of choice will be Solidity for our L1 contracts, so most of the specific style rules will be having that in mind, but the process for writing it can be adapted slightly for vyper, and will have an extension when Aztec.nr contracts are more mature.

![](https://media.tenor.com/ry_sCXk6wH0AAAAC/pirates-caribbean-code.gif)

## Outline
When you plan to build a system, or write a contract to extend an existing one. This should be the go-to approach.
1. Write a specification.
1. Write a specification.
- Define how the system is planned to work, what trade-offs are made and why. Do this without writing an implementation! The spec should have a "TL;DR" or similar so people can quickly skim to see what you want to do and why.
2. Write a reference implementation of the spec, focus on readability
- Solidity contracts especially can be a pain in the neck to read if an implementation is optimized heavily.
3. Write extensive test for individual functions, integrations and scenarios
4. Add fuzz-testing when the bases are covered
5. Review reference implementation
6. Fix any issues that have been revealed by the review
6. Fix any issues that have been revealed by the review
7. Create an optimized implementation

# 1. Writing a spec
Expand All @@ -38,16 +38,16 @@ The spec should outline the dependencies of the contract (other protocols and to
2. Makes it more clear what types of actions the accounting needs to support, and if any are missing.

## 2. Writing a reference implementation
A reference implementation is a practical instantiation of the spec that should strive to maximize readability.
A reference implementation is a practical instantiation of the spec that should strive to maximize readability.
It should make it very clear what the purpose is, and how it achieves its goal, meaning that no heavy optimizations should be made. The reference is simply to be used as a good way to understand the code, and to build tests for. To make assessing the coverage of the code easier, ternary operators should be avoid, e.g., instead of `a > b ? a : b` please use explicit `if-else` branching.
Having a reference implementation, that can later be optimized, should also make it easier for auditors and external parties to give a first look at the protocol, e.g., if there is a flaw in the business logic, it should be possible to catch at this point.

## 3. Writing tests
Test to cover all the user intended flows should be written, as well as tests for failure cases and less intended flows.
Test to cover all the user intended flows should be written, as well as tests for failure cases and less intended flows.

To make the test useful for reviewers, a small comment describing it in easily understandable language should be added.

When the tests have been written, coverage should be ~100%. If not, put focus on the parts that are missing, is there some possible behavior that we have not thought about or what is the reasoning behind the missing coverage.
When the tests have been written, coverage should be ~100%. If not, put focus on the parts that are missing, is there some possible behavior that we have not thought about or what is the reasoning behind the missing coverage.

### 3.1 Happy Paths
A happy path is a series of actions that we expect users to do, or be part of, e.g., users using the protocol as expected.
Expand All @@ -64,23 +64,23 @@ For actions with user-controlled input amounts or where inner values depend on t
## 5. Review reference implementation
For every issue found, create a `github` issue. These should be grouped in one of the following groups `informational, low, medium, high`. Issues should be separate, but if the same problem have been spotted multiple places, e.g., off-by one for both deposit and withdraw, it can be grouped into one `issue`.

When reviewing, consult the references to have checklists to go through.
When reviewing, consult the references to have checklists to go through.

Also, is the contract planned to be used behind a proxy or directly? If behind a proxy, is there are deployment script that will initialize it, and does the upgrade mechanism work or is something missing.

## 6. Handle issues
For each of the `github` issue that have been created by the review, either a PR should be made that addresses the issue or it should be acknowledged.
For each of the `github` issue that have been created by the review, either a PR should be made that addresses the issue or it should be acknowledged.

## 7. Create optimized implementation
Before writing the optimized implementation, the tests should be extended to the point where no test is lacking. Also, all tests should pass for both sets.

### 7.1 Good places to start
- Variable packing
- Cached variables (if read from storage multiple times, cache the value and use it)
- Unchecked math for increment in values where it is known that it will never overflow.
- Unchecked math for increment in values where it is known that it will never overflow.

### 7.2 Gas-comparison for reference / optimized
For each of the changes that are made, please add an estimate on the gas savings. If gas is saved and it doesn't significantly increase the code-size or heavily degrades the readability
For each of the changes that are made, please add an estimate on the gas savings. If gas is saved and it doesn't significantly increase the code-size or heavily degrades the readability

## Reviews
There is a couple of different angles when reviewing smart contracts:
Expand Down Expand Up @@ -116,27 +116,27 @@ We generally strive to adhere to the [soliditylang guidelines](https://docs.soli

### Natspec

Natspec should be written for all functions (`internal` mainly for clarity). Use the `@notice` tag for general explanation of the function that a user should be able to understand and `@dev` for more developer specific information.
Natspec should be written for all functions (`internal` mainly for clarity). Use the `@notice` tag for general explanation of the function that a user should be able to understand and `@dev` for more developer specific information.

```solidity
/**
* @notice Inserts a new message into the Inbox
* @dev Emits `MessageSent` with data for easy access by the sequencer
* @param _recipient - The recipient of the message
* @param _content - The content of the message (application specific)
* @param _secretHash - The secret hash of the message (make it possible to hide when a specific message is consumed on L2)
* @param _publicContentHash - Hash of the public content of the message
* @param _privateContentHash - Hash of the private content of the message
* @return Hash of the sent message.
*/
function sendL2Message(
DataStructures.L2Actor memory _recipient,
bytes32 _content,
bytes32 _secretHash
bytes32 _publicContentHash,
bytes32 _privateContentHash
) external override(IInbox) returns (bytes32) {
```

### Solhint configuration

Many of these guidelines are enforced by solhint, with additions in https://github.com/LHerskind/solhint.
Many of these guidelines are enforced by solhint, with additions in https://github.com/LHerskind/solhint.

```json
{
Expand All @@ -163,4 +163,4 @@ Many of these guidelines are enforced by solhint, with additions in https://gith
"comprehensive-interface": ["error"]
}
}
```
```
8 changes: 6 additions & 2 deletions l1-contracts/src/core/interfaces/IFeeJuicePortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@ import {IERC20} from "@oz/token/ERC20/IERC20.sol";
import {IInbox} from "./messagebridge/IInbox.sol";

interface IFeeJuicePortal {
event DepositToAztecPublic(bytes32 indexed to, uint256 amount, bytes32 secretHash, bytes32 key, uint256 index);
event DepositToAztecPublic(
bytes32 indexed to, uint256 amount, bytes32 privateContentHash, bytes32 key, uint256 index
);
event FeesDistributed(address indexed to, uint256 amount);

function distributeFees(address _to, uint256 _amount) external;
function depositToAztecPublic(bytes32 _to, uint256 _amount, bytes32 _secretHash) external returns (bytes32, uint256);
function depositToAztecPublic(bytes32 _to, uint256 _amount, bytes32 _privateContentHash)
external
returns (bytes32, uint256);

// solhint-disable-next-line func-name-mixedcase
function UNDERLYING() external view returns (IERC20);
Expand Down
13 changes: 7 additions & 6 deletions l1-contracts/src/core/interfaces/messagebridge/IInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,15 @@ interface IInbox {
* @notice Inserts a new message into the Inbox
* @dev Emits `MessageSent` with data for easy access by the sequencer
* @param _recipient - The recipient of the message
* @param _content - The content of the message (application specific)
* @param _secretHash - The secret hash of the message (make it possible to hide when a specific message is consumed
* on L2)
* @param _publicContentHash - Hash of the public content of the message (application specific)
* @param _privateContentHash - Hash of the private content of the message (also application specific).
* @return The key of the message in the set and its compact cumulative index
*/
function sendL2Message(DataStructures.L2Actor memory _recipient, bytes32 _content, bytes32 _secretHash)
external
returns (bytes32, uint256);
function sendL2Message(
DataStructures.L2Actor memory _recipient,
bytes32 _publicContentHash,
bytes32 _privateContentHash
) external returns (bytes32, uint256);
// docs:end:send_l1_to_l2_message

/**
Expand Down
9 changes: 4 additions & 5 deletions l1-contracts/src/core/libraries/DataStructures.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,15 @@ library DataStructures {
* @notice Struct containing a message from L1 to L2
* @param sender - The sender of the message
* @param recipient - The recipient of the message
* @param content - The content of the message (application specific) padded to bytes32 or hashed if larger.
* @param secretHash - The secret hash of the message (make it possible to hide when a specific message is consumed on
* L2).
* @param publicContentHash - Hash of the public content of the message.
* @param privateContentHash - Hash of the private content of the message.
* @param index - Global leaf index on the L1 to L2 messages tree.
*/
struct L1ToL2Msg {
L1Actor sender;
L2Actor recipient;
bytes32 content;
bytes32 secretHash;
bytes32 publicContentHash;
bytes32 privateContentHash;
uint256 index;
}

Expand Down
4 changes: 2 additions & 2 deletions l1-contracts/src/core/libraries/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ library Errors {
// Inbox
error Inbox__ActorTooLarge(bytes32 actor); // 0xa776a06e
error Inbox__VersionMismatch(uint256 expected, uint256 actual); // 0x47452014
error Inbox__ContentTooLarge(bytes32 content); // 0x47452014
error Inbox__SecretHashTooLarge(bytes32 secretHash); // 0xecde7e2c
error Inbox__PublicContentHashTooLarge(bytes32 publicContentHash); // 0x86f81123
error Inbox__PrivateContentHashTooLarge(bytes32 privateContentHash); // 0x3d5f989b
error Inbox__BucketOutOfWindow(uint256 seq, uint256 current); // 0xfee255b7
error Inbox__Unauthorized(); // 0xe5336a6b
error Inbox__WouldOverwriteUnconsumedBucket(uint64 evictedBucketSeq); // 0x2eb49c6d
Expand Down
4 changes: 3 additions & 1 deletion l1-contracts/src/core/libraries/crypto/Hash.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ library Hash {
*/
function sha256ToField(DataStructures.L1ToL2Msg memory _message) internal pure returns (bytes32) {
return sha256ToField(
abi.encode(_message.sender, _message.recipient, _message.content, _message.secretHash, _message.index)
abi.encode(
_message.sender, _message.recipient, _message.publicContentHash, _message.privateContentHash, _message.index
)
);
}

Expand Down
14 changes: 7 additions & 7 deletions l1-contracts/src/core/messagebridge/FeeJuicePortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,28 @@ contract FeeJuicePortal is IFeeJuicePortal {
* @notice Deposit funds into the portal and adds an L2 message which can only be consumed publicly on Aztec
* @param _to - The aztec address of the recipient
* @param _amount - The amount to deposit
* @param _secretHash - The hash of the secret consumable message. The hash should be 254 bits (so it can fit in a
* Field element)
* @param _privateContentHash - Hash of the private content of the L1 to L2 message. The hash should be
* 254 bits (so it can fit in a Field element)
* @return - The key of the entry in the Inbox and its leaf index
*/
function depositToAztecPublic(bytes32 _to, uint256 _amount, bytes32 _secretHash)
function depositToAztecPublic(bytes32 _to, uint256 _amount, bytes32 _privateContentHash)
external
override(IFeeJuicePortal)
returns (bytes32, uint256)
{
// Preamble
DataStructures.L2Actor memory actor = DataStructures.L2Actor(L2_TOKEN_ADDRESS, VERSION);

// Hash the message content to be reconstructed in the receiving contract
bytes32 contentHash = Hash.sha256ToField(abi.encodeWithSignature("claim(bytes32,uint256)", _to, _amount));
// Hash the public message content to be reconstructed in the receiving contract
bytes32 publicContentHash = Hash.sha256ToField(abi.encodeWithSignature("claim(bytes32,uint256)", _to, _amount));

// Hold the tokens in the portal
UNDERLYING.safeTransferFrom(msg.sender, address(this), _amount);

// Send message to rollup
(bytes32 key, uint256 index) = INBOX.sendL2Message(actor, contentHash, _secretHash);
(bytes32 key, uint256 index) = INBOX.sendL2Message(actor, publicContentHash, _privateContentHash);

emit DepositToAztecPublic(_to, _amount, _secretHash, key, index);
emit DepositToAztecPublic(_to, _amount, _privateContentHash, key, index);

return (key, index);
}
Expand Down
31 changes: 19 additions & 12 deletions l1-contracts/src/core/messagebridge/Inbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,28 @@ contract Inbox is IInbox {
* @dev Emits `MessageSent` with data for easy access by the sequencer
*
* @param _recipient - The recipient of the message
* @param _content - The content of the message (application specific)
* @param _secretHash - The secret hash of the message (make it possible to hide when a specific message is consumed
* on L2)
* @param _publicContentHash - Hash of the public content of the message (application specific)
* @param _privateContentHash - Hash of the private content of the message. The private content is
* application-defined (e.g. a claim secret, or the L2 recipient plus a blinding value) and is required to consume
* the message on L2
*
* @return Hash of the sent message and its compact cumulative index.
*/
function sendL2Message(DataStructures.L2Actor memory _recipient, bytes32 _content, bytes32 _secretHash)
external
override(IInbox)
returns (bytes32, uint256)
{
function sendL2Message(
DataStructures.L2Actor memory _recipient,
bytes32 _publicContentHash,
bytes32 _privateContentHash
) external override(IInbox) returns (bytes32, uint256) {
require(uint256(_recipient.actor) <= Constants.MAX_FIELD_VALUE, Errors.Inbox__ActorTooLarge(_recipient.actor));
require(_recipient.version == VERSION, Errors.Inbox__VersionMismatch(_recipient.version, VERSION));
require(uint256(_content) <= Constants.MAX_FIELD_VALUE, Errors.Inbox__ContentTooLarge(_content));
require(uint256(_secretHash) <= Constants.MAX_FIELD_VALUE, Errors.Inbox__SecretHashTooLarge(_secretHash));
require(
uint256(_publicContentHash) <= Constants.MAX_FIELD_VALUE,
Errors.Inbox__PublicContentHashTooLarge(_publicContentHash)
);
require(
uint256(_privateContentHash) <= Constants.MAX_FIELD_VALUE,
Errors.Inbox__PrivateContentHashTooLarge(_privateContentHash)
);

// Compact cumulative message index: the zero-based position of this message in the Inbox's
// insertion order, equal to the number of messages inserted before it. It matches the streaming L1-to-L2 tree's
Expand All @@ -110,8 +117,8 @@ contract Inbox is IInbox {
DataStructures.L1ToL2Msg memory message = DataStructures.L1ToL2Msg({
sender: DataStructures.L1Actor(senderAddress, block.chainid),
recipient: _recipient,
content: _content,
secretHash: _secretHash,
publicContentHash: _publicContentHash,
privateContentHash: _privateContentHash,
index: index
});

Expand Down
Loading
Loading