-
Notifications
You must be signed in to change notification settings - Fork 29
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
contracts: implement calldata encryption in Solidity #436
Conversation
✅ Deploy Preview for oasisprotocol-sapphire-paratime ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
); | ||
|
||
return | ||
abi.encodePacked( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the output bytecode for this is going to be huge
expect(parsedCborUint).eq(MAX_SAFE_UINT128); | ||
expect(newOffset).eq(16 + 1); | ||
|
||
//expect(await contract.testUintRoundtrip(MAX_SAFE_UINT128)).eq(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CBOR encoding integers larger than 64bit seems to vary between libraries, some use tagging, others don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker right. I'm sure we'll get dev feedback if this is buggy?
expect(parsedCborUint).eq(MAX_SAFE_UINT128); | ||
expect(newOffset).eq(16 + 1); | ||
|
||
//expect(await contract.testUintRoundtrip(MAX_SAFE_UINT128)).eq(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker right. I'm sure we'll get dev feedback if this is buggy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I'm PITA, but this is tedious to review. Why not make life easier and split this PR into five:
- CBOR.sol
- actual calldata encryption PR
- OPL support for encrypted calldata
- HMAC_sha512_256
- 0.3.0 version bump (changelog & new release?)
if (len > 0x20) revert CBOR_Error_InvalidLength(len); | ||
|
||
assembly { | ||
value := mload(add(add(0x20, result), offset)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loads a 256bit word from an offset of a byte array, the first 256bits of the byte array is the length prefix, which is skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Maybe add a comment?
value := mload(add(add(0x20, result), offset)) | |
// Load value from the result array at position 32+offset. | |
value := mload(add(add(0x20, result), offset)) |
Unfortunately splitting a small PR into lots of even smaller minor PRs is also very tedious. I can re-commit them as individual commits within the PR (that would be slightly less tedious)? |
); | ||
} | ||
|
||
function encryptCallData(bytes memory in_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this and add a short example similar to other functions in Sapphire.sol.
// OPAD ^ IPAD, (OPAD = 0x5c) | ||
bytes32 constant HMAC_OPAD_XOR_IPAD = 0x6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a; | ||
|
||
function HMAC_sha512_256(bytes memory key, bytes memory message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this and add a short example similar to other functions in Sapphire.sol.
constructor() payable { // solhint-disable-line | ||
// Do nothing, but allow balances to be sent on construction. | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work? Same below.
constructor() payable { // solhint-disable-line | |
// Do nothing, but allow balances to be sent on construction. | |
} | |
// Do nothing, but allow balances to be sent on construction. | |
constructor() payable {} |
Fixes: #428
subcall_static
to perform subcall withstaticcall
, makes several Subcall methodsview
nonce
to Cipher for encryption (useful for tests & reproducibility)Re: CBOR, I split the CBOR related stuff into individual functions, as otherwise you start needing to deploy the whole
CBOR
library on its own an link it into a library, whereas using individual function they get pulled in as & when necessary.