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

Parse SafeTx numeric parameters to int #288

Merged
merged 1 commit into from
Jul 13, 2022
Merged

Parse SafeTx numeric parameters to int #288

merged 1 commit into from
Jul 13, 2022

Conversation

Uxio0
Copy link
Member

@Uxio0 Uxio0 commented Jul 5, 2022

  • Guard SafeTx from other numeric formats like float, Decimal or even str

@moisses89
Copy link
Member

Why this is necessary?

@Uxio0
Copy link
Member Author

Uxio0 commented Jul 5, 2022

Why this is necessary?

That's a good question.

EIP712 library is very picky, and does not accept different types than expected. I spent today a lot of time debugging an issue because a Decimal was passed to a SafeTx (default numeric type used by Django admin) instead of an int, so I thought it would be very useful to just parse everything to int to make sure we are handling the correct datatypes and not finding issues later when calling safe_tx_hash

- Guard SafeTx from other numeric formats like float, Decimal or even str
@coveralls
Copy link

coveralls commented Jul 5, 2022

Pull Request Test Coverage Report for Build 2617366441

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 91.007%

Totals Coverage Status
Change from base Build 2617348191: 0.2%
Covered Lines: 2955
Relevant Lines: 3247

💛 - Coveralls

self.gas_token = gas_token or NULL_ADDRESS
self.refund_receiver = refund_receiver or NULL_ADDRESS
self.signatures = signatures or b""
self._safe_nonce = safe_nonce
self._safe_nonce = safe_nonce and int(safe_nonce)
Copy link
Member

Choose a reason for hiding this comment

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

Could be in this case int(safe_nonce) directly instead safe_nonce and int(safe_nonce)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because it can be None, and you don't want to try int(None) 😉

@Uxio0 Uxio0 merged commit 2b1fef1 into master Jul 13, 2022
@Uxio0 Uxio0 deleted the fix-safe-tx branch July 13, 2022 12:48
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants