-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
e6d8d43
to
25932b2
Compare
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@0x4007 This task looks off to me. If there's no progress soon, I discontinue my work here on this PR. |
Can you post QA to prove it's working |
848738c
to
2159e3e
Compare
@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. |
2159e3e
to
c476b5f
Compare
There was a problem hiding this 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
Latest QAGenerated output:
|
Technical question: I was reading the related spec and it states:
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if (contextOrPayload.config.runId) { | |
} else { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runId: runId, |
There was a problem hiding this comment.
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
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()), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I dont think this needs to be long running. So it makes sense to make an API endpoint hosted on Cloudflare Workers. |
Latest QAOutput:
|
What about the footer config code? |
I think in order to save time we should:
|
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 |
Could you please provide more details or specifications for this task? |
It's in the middle of the spec. But maybe it makes sense to scope this for only the reward amounts to be returned |
Resolves #68