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

feat: add the permit-generator github action #69

Closed
wants to merge 47 commits into from

Conversation

hhio618
Copy link

@hhio618 hhio618 commented Sep 15, 2024

Resolves #68

USERNAME=$(echo $user_amount | jq -r 'keys[0]')
AMOUNT=$(echo $user_amount | jq -r '.[]')

# TODO: This will convert the username to it's unique user ID.
Copy link
Author

Choose a reason for hiding this comment

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

Is this desired to be in here?

Copy link
Member

Choose a reason for hiding this comment

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

We should save user id for audit/accounting reasons. It's confusing if they change their username and that's all we store

@hhio618
Copy link
Author

hhio618 commented Sep 19, 2024

@0x4007 This task looks off to me. If there's no progress soon, I discontinue my work here on this PR.

@0x4007
Copy link
Member

0x4007 commented Sep 23, 2024

Can you post QA to prove it's working

@hhio618 hhio618 force-pushed the development branch 7 times, most recently from 848738c to 2159e3e Compare September 29, 2024 19:03
@0x4007
Copy link
Member

0x4007 commented Sep 30, 2024

@whilefoo @rndquu @gentlementlegen I don't recall who worked on the NFT minting part but the tests are failing due to it, and it doesn't seem like any changes related to it were made in this pull.

@0x4007 0x4007 mentioned this pull request Oct 12, 2024
@hhio618 hhio618 requested a review from 0x4007 October 14, 2024 14:59
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Is the UUID being used? Otherwise everything looks fine @whilefoo rfc

@hhio618
Copy link
Author

hhio618 commented Oct 15, 2024

Latest QA

Github action run#11348946725

Generated output:

W3sidG9rZW5UeXBlIjoiRVJDMjAiLCJ0b2tlbkFkZHJlc3MiOiIweGU5MWQxNTNlMGI0MTUxOGEyY2U4ZGQzZDc5NDRmYTg2MzQ2M2E5N2QiLCJiZW5lZmljaWFyeSI6IjB4NjMyMTI4NkY5QjczZjQyN0M3MmUxZjlGMWJDNmIzZDI1ZUYwNjYwNSIsIm5vbmNlIjoiMTE3MTAxNzE4NTUwNjA4NTI0NzQyODgxNTg5NDQ3MzY3Mjc1ODc2MjY4Nzk0NDc5MDQwMjMwOTI5MDQzOTkyNjUxNDMzNjM4OTIyMDUiLCJkZWFkbGluZSI6IjExNTc5MjA4OTIzNzMxNjE5NTQyMzU3MDk4NTAwODY4NzkwNzg1MzI2OTk4NDY2NTY0MDU2NDAzOTQ1NzU4NDAwNzkxMzEyOTYzOTkzNSIsImFtb3VudCI6IjIwMDAwMDAwMDAwMDAwMDAwMDAwMCIsIm93bmVyIjoiMHhmMzlGZDZlNTFhYWQ4OEY2RjRjZTZhQjg4MjcyNzljZmZGYjkyMjY2Iiwic2lnbmF0dXJlIjoiMHhmM2MwNDViZTEzMTU4MTJkMmQyYmVlZDdmOGQ0YWZlOTQwODU4NTg4YTBiZTdjNjlkZWM4MWI0MThjMzkxOWExMmY2NDE5YTUxZTJkMzIyZDJhMGVjZmI2ZTkyMTdhYzBlMTcwM2RkNjhhZGE0N2ZhMzcxZTNlODQzYTRmNWQxNzFjIiwibmV0d29ya0lkIjoxMDB9XQ==

@gentlementlegen
Copy link
Member

Technical question: I was reading the related spec and it states:

This should be the only way to generate permits in our system, so that we can enforce capturing value from plugin developers.

So it would mean that https://github.com/ubiquity-os-marketplace/text-conversation-rewards for example should be using this instead of generating permits on its own. It could eventually call this Action, but workers are not instantaneous and the end of the workflow should be awaited somewhere. In the case of a Worker that would eventually need to generate a permit, I don't see how this could be called either. Wouldn't it make more sense to have this as an endpoint? I understand this is a long running task which is why it was designed as an Action, but seems tricky to use in a real-case scenario.

context.adapters = createAdapters(supabaseClient, context);

const permits = await generatePayoutPermit(context, config.permitRequests);
await returnDataToKernel(env.GITHUB_TOKEN, "todo_state", permits);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why you are returning data to kernel if this workflow is manually triggered by someone

Copy link
Author

Choose a reason for hiding this comment

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

I initially thought it was meant to be part of the process. Should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because this script is only called manually and not by the kernel

@@ -54,6 +55,8 @@ export async function generateErc20PermitSignature(
issueNodeId = contextOrPayload.payload.issue.node_id;
} else if ("pull_request" in contextOrPayload.payload) {
issueNodeId = contextOrPayload.payload.pull_request.node_id;
} else if (contextOrPayload.config.runId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (contextOrPayload.config.runId) {
} else {

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review, removed.

evmNetworkId: Number(env.EVM_NETWORK_ID),
evmPrivateEncrypted: env.EVM_PRIVATE_KEY,
permitRequests: permitRequests,
runId: runId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
runId: runId,

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

src/types/env.ts Outdated
Comment on lines 11 to 14
EVM_NETWORK_ID: T.Optional(T.String()),
EVM_PRIVATE_KEY: T.Optional(T.String()),
EVM_TOKEN_ADDRESS: T.Optional(T.String()),
PAYMENT_REQUESTS: T.Optional(T.String()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a way to avoid cluttering the env just for this Action, maybe through CLI?

Copy link
Author

Choose a reason for hiding this comment

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

Certainly, I've taken care of that in the following commits.

@0x4007
Copy link
Member

0x4007 commented Oct 16, 2024

I understand this is a long running task which is why it was designed as an Action, but seems tricky to use in a real-case scenario.

I dont think this needs to be long running. So it makes sense to make an API endpoint hosted on Cloudflare Workers.

@hhio618
Copy link
Author

hhio618 commented Oct 16, 2024

Latest QA

Github action run#11379446214

Output:

W3sidG9rZW5UeXBlIjoiRVJDMjAiLCJ0b2tlbkFkZHJlc3MiOiIweGU5MWQxNTNlMGI0MTUxOGEyY2U4ZGQzZDc5NDRmYTg2MzQ2M2E5N2QiLCJiZW5lZmljaWFyeSI6IjB4NjMyMTI4NkY5QjczZjQyN0M3MmUxZjlGMWJDNmIzZDI1ZUYwNjYwNSIsIm5vbmNlIjoiNzY1OTI2OTEzNjcyMjY0NDcxNjk2NzA2NDQ1MTk2MDM0ODA3ODExMDY1Nzc2MzMxMjMwNjkyNjM5NzcyODk2NzQwMDk2MDA2Mjc0ODIiLCJkZWFkbGluZSI6IjExNTc5MjA4OTIzNzMxNjE5NTQyMzU3MDk4NTAwODY4NzkwNzg1MzI2OTk4NDY2NTY0MDU2NDAzOTQ1NzU4NDAwNzkxMzEyOTYzOTkzNSIsImFtb3VudCI6IjIwMDAwMDAwMDAwMDAwMDAwMDAwMCIsIm93bmVyIjoiMHhmMzlGZDZlNTFhYWQ4OEY2RjRjZTZhQjg4MjcyNzljZmZGYjkyMjY2Iiwic2lnbmF0dXJlIjoiMHhiNmY2NjA5YjRiOGI4ZjViMjYwZDRhODc4ZmIyY2FlYzk2MjMwZWJmNDlmZmVhZjVkNzZkNDRiMjYxYmZhZjM1MjUxYzIxM2RkNTdhNmFhMjU1MDlhMGUwZmQxYTcxNDBjOTcyOTVjM2U0MDk4YzYzZjE0ZWVlZjhjYjc3NzQ2YTFjIiwibmV0d29ya0lkIjoxMDB9XQ==

@hhio618 hhio618 requested a review from whilefoo October 17, 2024 06:23
@hhio618 hhio618 requested a review from 0x4007 October 19, 2024 05:56
@0x4007
Copy link
Member

0x4007 commented Oct 20, 2024

What about the footer config code?

@rndquu
Copy link
Member

rndquu commented Oct 21, 2024

I think in order to save time we should:

  1. Close this PR (because many changes are incompatible with the cloudflare worker code)
  2. Close Convert to "Plugin" #68 as completed
  3. Proceed straight to Convert plugin to cloudflare worker #92

@rndquu
Copy link
Member

rndquu commented Oct 21, 2024

What about the footer config code?

The actual comment is posted via https://github.com/ubiquity-os-marketplace/text-conversation-rewards, https://github.com/ubiquity-os/permit-generation is responsible solely for permits

@hhio618
Copy link
Author

hhio618 commented Oct 22, 2024

What about the footer config code?

Could you please provide more details or specifications for this task?

@0x4007
Copy link
Member

0x4007 commented Oct 22, 2024

It's in the middle of the spec. But maybe it makes sense to scope this for only the reward amounts to be returned

@0x4007 0x4007 closed this Oct 22, 2024
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.

Convert to "Plugin"
7 participants