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/workerize #96

Draft
wants to merge 131 commits into
base: development
Choose a base branch
from
Draft

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Oct 30, 2024

Resolves #92

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 10, 2024

Can the reward be increased on this task to $1200 to match the issue that was just closed that this actually resolves please? That would be great

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 17, 2024

@rndquu @gentlementlegen @zugdev @0x4007

I'm assuming that this cannot be reviewed and merged until the rest of the infra has been built and ready to merge?

Tasks like ubiquity-os-marketplace/text-conversation-rewards#64 (comment) by the looks of it will be changing the current permit encoding schema so can review be started on this or is this on hold until other infra is ready?

@gentlementlegen
Copy link
Member

@Keyrxng you should assign people for review I forgot about this 😄 But I think it is testable. Eventually it will induce changes in plugins like text-conversation-rewards but can exist on its own, will test it.

@gentlementlegen
Copy link
Member

@Keyrxng I know there are already a lot of changes, but I think it should be worth adding the SDK here (which is basically the changes you made but manually).

Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 22, 2024

@Keyrxng you should assign people for review I forgot about this

Lmao it's still not possible to request review from anyone, think there is a settings issue or something idk.

@Keyrxng I know there are already a lot of changes, but I think it should be worth adding the SDK here (which is basically the changes you made but manually).

Hmmmm idk because this needs to function as both a plugin and a standalone worker, is the SDK that extensible yet? I can try it though and we'll see I guess.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 22, 2024

https://github.com/ubiquity-os/plugin-sdk/blob/004e1735a6c98d7bb015ac649bb2bbb57890f48f/src/server.ts#L32

The signature was my concern and I see we can bypass it so that's cool but not enough to support this worker I don't think.

https://github.com/ubiquity-os/plugin-sdk/blob/004e1735a6c98d7bb015ac649bb2bbb57890f48f/src/server.ts#L72

How it's bypassed would mean that we'd bypass all requests but we'd still like to enforce verification if it's a non-UI request I'd imagine. Which would mean I'd need to create two hono instances one to verify GH sided reqs and one to bypass UI reqs and implement some other handshake.

What do you think @gentlementlegen?

Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

.dev.vars.example Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member

gentlementlegen commented Nov 27, 2024

@Keyrxng Can you please resolve the conflicts? Also everything should be changed to use bun as a package manager for simplicity.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 27, 2024

please resolve the conflicts? Also everything should be changed to use bun as a package manager for simplicity.

Conflicts resolved my friend.

Can we create another task to migrate everything to bun please as this has been sat for a long time ready and you can make a clear spec on a bun migration for me as idk what that entails off the top of my head and I know it can also affect tests given what you've said on it before in regards to the kernel

Also knip is having another meltdown it seems

Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 29, 2024

What do you think @gentlementlegen, you okay with a new task for a bun migration or is it a requirement for this PR to move forward?

@gentlementlegen
Copy link
Member

It is fine to do it in another task although it should be quick I think.

Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

@rndquu rndquu requested review from gentlementlegen and rndquu and removed request for rndquu December 20, 2024 09:45
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

  1. Somehow this request:
{
    "settings": {
        "evmPrivateEncrypted": "INVALID",
        "permitRequests": [
            {
                "kind": "ERC721",
                "type": "ERC721",
                "userId": 119500907,
                "amount": 1,
                "evmNetworkId": 100,
                "tokenAddress": "0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d",
                "issueNodeId": "0x123",
                "erc721Request": {
                    "contributionType": "issue.closed",
                    "keys": [
                        "GITHUB_ORGANIZATION_NAME", 
                        "GITHUB_REPOSITORY_NAME", 
                        "GITHUB_ISSUE_NODE_ID", 
                        "GITHUB_USERNAME", 
                        "GITHUB_CONTRIBUTION_TYPE"
                    ],
                    "values": ["test", "test", "123", "123", "contribution"],
                    "metadata": {
                        "GITHUB_CONTRIBUTION_TYPE": "contribution",
                        "GITHUB_ISSUE_NODE_ID": "123",
                        "GITHUB_ORGANIZATION_NAME": "test",
                        "GITHUB_REPOSITORY_NAME": "test",
                        "GITHUB_USERNAME": "123"
                    }
                }
            }
        ]
    }
}

returns this signed response:

[
    {
        "tokenType": "ERC721",
        "tokenAddress": "0x0000000000000000000000000000000000000003",
        "beneficiary": "0x336C033842FA316d470e820c81b742e62A0765DC",
        "amount": 1,
        "nonce": "24409459659634628035242948860406449062914951070652615187495949183624535261750",
        "deadline": "115792089237316195423570985008687907853269984665640564039457584007913129639935",
        "signature": "0xb992feeb3fc8488a162d232fada403a55b4f0bf6150dba4bfea266b3850a27df62a61ef029cdd09c3d87016283a774f51de1e2fe235b970297bb45ebe05f8d071c",
        "owner": "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266",
        "networkId": 100,
        "erc721Request": {
            "keys": [
                "0x3e4c8b8ab7e90cb1a6f2a4f809d6709c7eb62a413bdfb84f966832a075f4e4e4",
                "0x1f8205ede46f9b1cd2bb5d0f0d18e693848acc572c08bd312de5b83c0903cde0",
                "0x3aea1069a51bcd90299edb08a451733591bb49d89d05ab061f7933673466ccee",
                "0x6ead3148a7e9ddc486a0ca0542fa4a73f19558feda0b070d41a5c80573eeb9dc",
                "0x1f3d3cb9dfb4f8b84afce5bab9ed497e4967e37149f9045f96de46cfa7ad0c6f"
            ],
            "values": [
                "test",
                "test",
                "0x123",
                "123",
                "contribution"
            ],
            "metadata": {
                "GITHUB_ORGANIZATION_NAME": "test",
                "GITHUB_REPOSITORY_NAME": "test",
                "GITHUB_ISSUE_NODE_ID": "0x123",
                "GITHUB_USERNAME": "123",
                "GITHUB_CONTRIBUTION_TYPE": "contribution"
            }
        }
    }
]

although the evmPrivateEncrypted param is clearly invalid. The ERC20 permit generation throws the Failed to decrypt a private key: Error: invalid input error as expected in this case. Pls fix and add a unit test catching the Failed to decrypt a private key error on generating ERC721.

  1. Pls fix failing knip CI

@rndquu rndquu marked this pull request as draft December 20, 2024 11:02
const permitRequestSchema = T.Union([
T.Object({
kind: T.Literal("ERC20"), // Discriminant key
type: T.Literal("ERC20"), // should remove this ideally
Copy link
Member

Choose a reason for hiding this comment

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

Aren't the kind and type properties the same? Can we remove the kind property? I can't find it in the latest development branch.

Copy link
Contributor Author

@Keyrxng Keyrxng Dec 20, 2024

Choose a reason for hiding this comment

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

Typebox needs kind so we should remove type if anything

But yeah they are exactly the same, I kept just to get an opinion on the matter.

Copy link
Member

Choose a reason for hiding this comment

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

Typebox needs kind

I don't understand. The kind property is missing in the latest development branch of https://github.com/ubiquity-os/permit-generation. Why is it appeared suddenly?

Copy link
Contributor Author

@Keyrxng Keyrxng Dec 20, 2024

Choose a reason for hiding this comment

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

I have added it in this PR to create a Typebox union (Typebox needs kind and doesn't care about type) so that we are handling the two different permits type more efficiently than what we were doing before. Now the permit inputs are being validated statically and at runtime.

Can simplify it back to what we had before if need be

Copy link
Member

Choose a reason for hiding this comment

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

Possible values for type are ERC20 and ERC721.

What are the possible values for the kind property?

Copy link
Contributor Author

@Keyrxng Keyrxng Dec 20, 2024

Choose a reason for hiding this comment

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

Exactly the same, they are the same prop with a different name.

I was disappointed I couldn't assign type as the discrim key and had to use kind.

Effectively they are identical in terms of value and application. Typebox just uses kind to pick out which of the union members we are referring to

Copy link
Member

Choose a reason for hiding this comment

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

@gentlementlegen help. I still can't understand why we need 2 exactly same properties here.

Copy link
Contributor Author

@Keyrxng Keyrxng Dec 20, 2024

Choose a reason for hiding this comment

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

we don't @rndquu.

I'm removing type

This PR will required any infra that touches this to be refactored anyway, so it is not a problem in that sense.

I kept it to have a discussion regarding swapping the two props. I asked @gentlementlegen but he never got backed to me about it, so I kept both and added code comments so review would see it

Copy link
Member

Choose a reason for hiding this comment

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

kind should not be needed technically, it is internal to typebox. I do not have the full context right now so I can't give you a proper answer but only one should be needed (and it is probably type).

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 plugin to cloudflare worker
5 participants