Skip to content

export generateAccessToken util functions#41

Open
ra-phael wants to merge 2 commits intomainfrom
feat/export-generateAccessToken
Open

export generateAccessToken util functions#41
ra-phael wants to merge 2 commits intomainfrom
feat/export-generateAccessToken

Conversation

@ra-phael
Copy link
Contributor

@ra-phael ra-phael commented May 29, 2023

Exporting the generateAccessToken util functions will avoid code duplication: this is happening in swap-router-contracts where they've been copied ( https://github.com/violetprotocol/mauve-swap-router-contracts/blob/main/test/shared/generateAccessToken.ts )
Once re-published, another PR will follow in mauve-swap-router-contracts to import these functions.

Export the roles as bytes32 as well, for the same reason.

@ra-phael ra-phael requested a review from 0xpApaSmURf May 30, 2023 15:30
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this might be better in a constants folder since it's not really a util?

"url": "https://github.com/violetprotocol/mauve-periphery"
},
"files": [
"utils/**/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we intending to use this? There are some seemingly small problems that could arise if we use this package outside of testing modules, since the generateAccessToken stuff here was originally written as a test utility.

Maybe it's even worth putting some code in the generateAccessToken function itself to throw if we're not in a testing state to disallow usage of the function in dev or prod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to be used for testing in the swap-router-contracts repo as well.

What issues do you see could arise?

Copy link
Contributor

Choose a reason for hiding this comment

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

We wouldn't want this to be used by any kind of real EAT generation, which is possible if it's imported. Since we have a default expiry that's hardcoded you could end up with generating EATs with completely wrong expiries which could be really dangerous.

Feels safer to disable the function to be called by anything other than tests.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants