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

Hotfix: Remove Payload Reversal #81

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Hotfix: Remove Payload Reversal #81

merged 1 commit into from
Jul 29, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Jul 29, 2024

User description

Something that was somehow missed in #65 is that payloads are no longer reversed. This fixes that and updates tests.


PR Type

Bug fix, Tests


Description

  • Removed reversal of payload in requestSignature method in NearEthAdapter class.
  • Removed reversal of payload in toPayload function in transaction utility.
  • Updated test cases in utils.transaction.test.ts to reflect payload without reversal.
  • Updated test cases in wc.handlers.test.ts to reflect payload without reversal.

Changes walkthrough 📝

Relevant files
Bug fix
ethereum.ts
Remove payload reversal in Ethereum adapter                           

src/chains/ethereum.ts

  • Removed reversal of payload in requestSignature method.
+1/-1     
transaction.ts
Remove payload reversal in transaction utility                     

src/utils/transaction.ts

  • Removed reversal of payload in toPayload function.
+1/-1     
Tests
utils.transaction.test.ts
Update transaction utility tests for payload change           

tests/unit/utils.transaction.test.ts

  • Updated test cases to reflect payload without reversal.
+2/-2     
wc.handlers.test.ts
Update Wallet Connect handler tests for payload change     

tests/unit/wc.handlers.test.ts

  • Updated test cases to reflect payload without reversal.
+12/-12 

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

@bh2smith
Copy link
Collaborator Author

We shouldn't have had two places where the payload was being reversed. Should investigate how to combine these.

@mintbase-codium-pr-agent
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No key issues to review

@bh2smith bh2smith merged commit 0f719ae into main Jul 29, 2024
1 check passed
@bh2smith bh2smith deleted the remove-payload-reverse branch July 29, 2024 12:19
@mintbase-codium-pr-agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Add error handling for the asynchronous requestSignature method call

Consider adding error handling for the requestSignature method call to handle
possible rejections or failures, especially since it involves asynchronous
operations.

src/chains/ethereum.ts [237-241]

-const signature = await this.mpcContract.requestSignature({
-  path: this.derivationPath,
-  payload: Array.from(hashToSign),
-  key_version: 0,
-});
+let signature;
+try {
+  signature = await this.mpcContract.requestSignature({
+    path: this.derivationPath,
+    payload: Array.from(hashToSign),
+    key_version: 0,
+  });
+} catch (error) {
+  console.error('Failed to request signature:', error);
+  throw error; // or handle error as appropriate
+}
 
Suggestion importance[1-10]: 9

Why: Adding error handling for the requestSignature method call is crucial for robustness, especially since it involves asynchronous operations that can fail. This suggestion significantly improves the reliability of the code.

9
Enhancement
Validate the format of hexString to ensure it starts with '0x'

Validate the input hexString to ensure it starts with '0x' before processing, to
prevent errors when slicing and converting to bytes.

src/utils/transaction.ts [13-16]

+if (!hexString.startsWith('0x')) {
+  throw new Error(`Invalid hex string format: ${hexString}`);
+}
 if (hexString.slice(2).length !== 32 * 2) {
   throw new Error(`Payload Hex must have 32 bytes: ${hexString}`);
 }
 return Array.from(toBytes(hexString));
 
Suggestion importance[1-10]: 8

Why: Ensuring the hexString starts with '0x' before processing prevents potential errors and improves input validation. This is a valuable enhancement for data integrity.

8
Maintainability
Use a utility function to generate expected payload arrays for test assertions

Consider using a utility function to generate expected payload arrays from hex
strings to reduce redundancy and improve maintainability of tests.

tests/unit/utils.transaction.test.ts [13-16]

-expect(payload).toEqual([
-  179, 145, 88, 233, 130, 44, 7, 211, 98, 140, 132, 230, 199, 101, 9, 36,
-  37, 94, 214, 13, 217, 70, 225, 215, 212, 59, 210, 203, 239, 90, 243, 178,
-]);
+const expectedPayload = generateExpectedPayload(txHash);
+expect(payload).toEqual(expectedPayload);
 
Suggestion importance[1-10]: 7

Why: Using a utility function to generate expected payload arrays reduces redundancy and improves maintainability of the tests. However, the improvement is more about code cleanliness than fixing a critical issue.

7
Refactor payload expectation checks into a shared function for better code reuse and readability

Refactor the repeated payload expectation checks into a shared function to improve
code reuse and test readability.

tests/unit/wc.handlers.test.ts [27-31]

-expect(payload).toEqual([
-  82, 182, 67, 125, 181, 109, 135, 245, 153, 29, 124, 23, 60, 241, 27,
-  157, 208, 249, 251, 8, 50, 96, 190, 241, 191, 12, 51, 128, 66, 188, 57,
-  140,
-]);
+expectPayloadToMatch(payload, expectedPayloadValues);
 
Suggestion importance[1-10]: 7

Why: Refactoring repeated payload expectation checks into a shared function enhances code reuse and readability, which is beneficial for maintainability. This suggestion improves the test code structure but is not critical.

7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant