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

add verifyingContract parameter to create permit2 signed message. #48

Closed
wants to merge 2 commits into from

Conversation

freereaper
Copy link

dex on other chain use different permit2 address.

dex on other chain use different permit2 address.
@NBFinanceTech
Copy link

Hello
line 78, the comma "," is incorrect ,

dex on other chain use different permit2 address.
@freereaper
Copy link
Author

thanks, have fixed the typo.

@Elnaril
Copy link
Owner

Elnaril commented Mar 29, 2024

Thank you for your PR!
Indeed, it make sense to have the verifyingContract configurable for UR/Permit2 forks.

Your changes look rather good to me, though a few refinements would be necessary to keep the lib consistent:

  • as a parameter, verifyingContract should follow the PEP 8 naming convention and be verifying_contract. (of course the corresponding key in the structured_data dict remains verifyingContract! :) )
  • verifying_contract should be a ChecksumAddress, not just a str
  • the permit2 address should be defined with the other constants in _constants.py and imported into router_codec.py, something like:
# in _constants.py
from web3 import Web3

permit2_address = Web3.to_checksum_address("0x000000000022D473030F116dDEE9F6B43aC78BA3")

# and in `_structured_data_permit`
'verifyingContract': permit2_address,
  • this new verifying_contract parameter should be documented in the method docstring. Basically, a short sentence to explain what it is and what is its default value.

Also, make sure the changes pass all the tests, including the linting.

Thanks again!

@freereaper
Copy link
Author

Thanks for your time, I have submitted a new PR.

@Elnaril
Copy link
Owner

Elnaril commented Apr 1, 2024

Closing because done by #52

@Elnaril Elnaril closed this Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants