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

contracts: implement HMAC SHA512-256, as it's used throughout the Oasis ecosystem #439

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

CedarMist
Copy link
Member

@CedarMist CedarMist commented Oct 15, 2024

No description provided.

@CedarMist CedarMist added the contracts Pull requests that update sapphire-contracts label Oct 15, 2024
@CedarMist CedarMist requested a review from matevz October 15, 2024 13:27
@CedarMist CedarMist self-assigned this Oct 15, 2024
Copy link

netlify bot commented Oct 15, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 60ec07d
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/6717aa6125598f00081c1b95

@CedarMist CedarMist force-pushed the CedarMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256 branch 2 times, most recently from ccbed9c to 2b220d3 Compare October 15, 2024 14:48
@CedarMist CedarMist requested review from aefhm and rube-de October 15, 2024 14:50
@CedarMist CedarMist force-pushed the CedarMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256 branch from 2b220d3 to 92b3606 Compare October 15, 2024 14:56
matevz
matevz previously approved these changes Oct 15, 2024
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
@CedarMist
Copy link
Member Author

CedarMist commented Oct 15, 2024

@matevz I forgot to run format on the code after adding comments.

For clarity, which style do you prefer:

diff --git a/contracts/contracts/HMAC_sha512_256.sol b/contracts/contracts/HMAC_sha512_256.sol
index 119f907..06becfc 100644
--- a/contracts/contracts/HMAC_sha512_256.sol
+++ b/contracts/contracts/HMAC_sha512_256.sol
@@ -51,12 +51,12 @@ function HMAC_sha512_256(bytes memory key, bytes memory message)
             let size := mload(key)
             // Call the identity precompile to copy memory
             success := staticcall(
-                gas(),           // Forward all available gas
-                PRECOMPILE_IDENTITY_ADDRESS,  // Address of the identity precompile
-                add(32, key),    // Start of the key data (skip the length prefix)
-                size,            // Length of data to copy
-                buf,             // Destination to copy to
-                size             // Amount of memory to copy
+                gas(), // Forward all available gas
+                PRECOMPILE_IDENTITY_ADDRESS, // Address of the identity precompile
+                add(32, key), // Start of the key data (skip the length prefix)
+                size, // Length of data to copy
+                buf, // Destination to copy to
+                size // Amount of memory to copy
             )
         }
 

@CedarMist CedarMist requested a review from matevz October 15, 2024 15:27
@CedarMist CedarMist dismissed matevz’s stale review October 15, 2024 15:28

Don't think it's a good idea to merge if there have been changes since the last review

@CedarMist
Copy link
Member Author

The auth tests are still causing intermittent failures

contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Comments are too verbose IMO. Otherwise looks good.

contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/contracts/HMAC_sha512_256.sol Outdated Show resolved Hide resolved
contracts/test/hashes.ts Show resolved Hide resolved
@CedarMist CedarMist marked this pull request as draft October 21, 2024 18:37
@CedarMist CedarMist force-pushed the CedarMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256 branch from 2d23aac to 3433438 Compare October 21, 2024 19:37
@CedarMist
Copy link
Member Author

CedarMist commented Oct 21, 2024

SIWE auth tests are still intermittently failing

@CedarMist CedarMist requested review from rube-de and matevz October 21, 2024 19:49
@matevz
Copy link
Member

matevz commented Oct 22, 2024

SIWE auth tests are still intermittently failing

I think I found the issue #443. Will fix it

Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Nice!

@CedarMist CedarMist marked this pull request as ready for review October 22, 2024 13:36
@CedarMist CedarMist force-pushed the CedarMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256 branch from 3433438 to 60ec07d Compare October 22, 2024 13:36
@CedarMist CedarMist merged commit 77c1abe into main Oct 23, 2024
11 checks passed
@CedarMist CedarMist deleted the CedarMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256 branch October 23, 2024 02:49
github-actions bot added a commit that referenced this pull request Oct 23, 2024
…edarMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256

contracts: implement HMAC SHA512-256, as it's used throughout the Oasis ecosystem 77c1abe
github-actions bot added a commit that referenced this pull request Oct 23, 2024
…darMist/1-of-many-PRs-for-matevz-HMAC-SHA512-256

contracts: implement HMAC SHA512-256, as it's used throughout the Oasis ecosystem 77c1abe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Pull requests that update sapphire-contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants