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.

Sign up for GitHub

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

Jump to bottom

feat: generate erc20 / erc721 permit #4

Merged
merged 11 commits into from
Apr 5, 2024
Merged

feat: generate erc20 / erc721 permit #4

merged 11 commits into from
Apr 5, 2024

Conversation

gentlementlegen
Copy link
Member

@gentlementlegen gentlementlegen commented Apr 3, 2024 •
edited
Loading

Based on PR #2
Aims to enable ERC 20 / 721 permit generation.

@gentlementlegen gentlementlegen requested a review from 0x4007 April 3, 2024 10:29
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 3, 2024

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

@0x4007 0x4007 removed their request for review April 3, 2024 10:33
@0x4007
Copy link
Member

0x4007 commented Apr 3, 2024

Make sure to file an issue to handle erc721 stuff later. It's not a top priority and we are behind schedule.

@gentlementlegen gentlementlegen changed the title feat: generate erc20 permit feat: generate erc20 / erc720 permit Apr 3, 2024
@gentlementlegen gentlementlegen changed the title feat: generate erc20 / erc720 permit feat: generate erc20 / erc721 permit Apr 3, 2024
@molecula451
Copy link

ready to review? @gentlementlegen

@gentlementlegen gentlementlegen marked this pull request as ready for review April 3, 2024 18:39
@gentlementlegen
Copy link
Member Author

gentlementlegen commented Apr 3, 2024 •
edited
Loading

@molecula451 was trying to get tests working first but they are using bun which doesn't play well with jest imports. Review should be ready, I'll try to get a run on my repo and link it here instead.

A successful run can be found at https://github.com/gentlementlegen/permit-generation/actions/runs/8544054410/job/23409249851#step:4:84

gitcoindev
gitcoindev reviewed Apr 3, 2024
View reviewed changes
.github/workflows/compute.yml Show resolved Hide resolved
src/handlers/generate-erc20-permit.ts Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member Author

Thanks for the review team. Latest run: https://github.com/gentlementlegen/permit-generation/actions/runs/8549616506/job/23425247355

@0x4007
Copy link
Member

0x4007 commented Apr 4, 2024

Seems good enough. Lets test in "production" aka lets run the kernel without any real funds, concurrently, with our old bot instance to compare performance/results.

When this is merged, will the CI be fixed?

@gentlementlegen
Copy link
Member Author

@pavlovcik Yep CI should be, because right now CI is running with the file contained within the repo development branch not with my changes on this branch.

@0x4007
Copy link
Member

0x4007 commented Apr 4, 2024

@molecula451 @gitcoindev please review as soon as possible! Thank you.

molecula451
molecula451 approved these changes Apr 4, 2024
View reviewed changes
Copy link

@molecula451 molecula451 left a comment

Choose a reason for hiding this comment

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

looks idiomatic, i'm approving

@gentlementlegen
Copy link
Member Author

New latest run after changes
https://github.com/gentlementlegen/permit-generation/actions/runs/8566692541/job/23476937894

@gentlementlegen gentlementlegen mentioned this pull request Apr 5, 2024
Merged
gitcoindev
gitcoindev suggested changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@gitcoindev gitcoindev left a comment

Choose a reason for hiding this comment

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

Just a few questions, after @gentlementlegen will answer I will approve and merge.

src/handlers/generate-erc20-permit.ts Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
package.json Show resolved Hide resolved
gitcoindev
gitcoindev approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@gitcoindev gitcoindev left a comment

Choose a reason for hiding this comment

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

Approving, all comments resolved.

@gitcoindev gitcoindev merged commit 984477a into ubiquity-os:development Apr 5, 2024
0 of 2 checks passed
@molecula451
Copy link

i thought gitcoindev had fixed knip? why no deprecated the feature if it's not going to fix?

@gitcoindev
Copy link
Contributor

gitcoindev commented Apr 5, 2024 •
edited
Loading

i thought gitcoindev had fixed knip? why no deprecated the feature if it's not going to fix?

hi @molecula451 Knip pull request was not merged yet, I added you to reviewers: ubiquity/keygen.ubq.fi#5 (review) . First Knip fix goes to keygen repo, then after approval I will clone and fix to all other repositories.

@gentlementlegen gentlementlegen deleted the generate-erc20-permit branch April 6, 2024 02:37
Keyrxng pushed a commit to ubq-testing/permit-generation that referenced this pull request Oct 30, 2024
@gentlementlegen
Merge pull request ubiquity-os#4 from Keyrxng/main
cf32c8f 
Update workflow trigger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x4007 0x4007 0x4007 approved these changes

@gitcoindev gitcoindev gitcoindev approved these changes

@molecula451 molecula451 molecula451 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gentlementlegen @0x4007 @molecula451 @gitcoindev @whilefoo