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

Permits #2

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Permits #2

merged 1 commit into from
Apr 5, 2024

Conversation

whilefoo
Copy link
Member

No description provided.

Copy link
Contributor

Coverage Report (0%) 
File% Stmts% Branch% Funcs% LinesUncovered Line #s

@@ -40,7 +37,7 @@
"blake2b": "^2.1.4",
"decimal.js": "^10.4.3",
"dotenv": "^16.4.4",
"ethers": "^5.7.2",
"ethers": "^6.11.1",
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change and I recall that it really changes the API I hope you did this for good reason!

Copy link
Member

Choose a reason for hiding this comment

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

5.x seems to be marked legacy so eventually we should bump it up but right now that breaks the code, as it is in the latest commit of that branch.

const { type, amount, username, contributionType } = permitRequest;

let permit: Permit;
switch (type) {
Copy link
Member

Choose a reason for hiding this comment

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

Why switch case for this

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine, if eventually we want to have more permits? It's always better to have a switch case than an if 🌲 forest.

Copy link
Member

Choose a reason for hiding this comment

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

We don't have any plans for different types of permits

@@ -40,7 +37,7 @@
"blake2b": "^2.1.4",
"decimal.js": "^10.4.3",
"dotenv": "^16.4.4",
"ethers": "^5.7.2",
"ethers": "^6.11.1",
Copy link
Member

Choose a reason for hiding this comment

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

5.x seems to be marked legacy so eventually we should bump it up but right now that breaks the code, as it is in the latest commit of that branch.

@@ -30,7 +30,7 @@ jobs:
node-version: "20.10.0"

- name: Install dependencies
run: yarn
run: npm install -g bun && bun install
Copy link
Member

Choose a reason for hiding this comment

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

This can directly be used within https://bun.sh/guides/runtime/cicd
However I don't see where bun is used?

const { type, amount, username, contributionType } = permitRequest;

let permit: Permit;
switch (type) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine, if eventually we want to have more permits? It's always better to have a switch case than an if 🌲 forest.

@gitcoindev gitcoindev merged commit 3b928a1 into development Apr 5, 2024
1 of 5 checks passed
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.

4 participants