-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix: update config types to match the new bot config #154
Conversation
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 realized we didn't clearly document something that is crucial for you to know. The production deployment of the bot is actually in a totally different/unexpected place because we were in a rush to demo it for some conference a few months ago. It's on my fork and its on the latest branch there which I have to find again (or on desktop you can see the latest branches)
I still need to merge it back in but at the same time we started work on redoing the architecture completely to "kernel-plugin" style over at ubiquibot-kernel repo under ubiquity organization.
The net result is that we plan to prototype the event handlers in this monolithic repository since it's semi functional and then port them over into separate plugins to interface with the kernel when it's stable.
@@ -5,3 +5,7 @@ | |||
[submodule "lib/permit2"] | |||
path = lib/permit2 | |||
url = https://github.com/Uniswap/permit2 | |||
[submodule "lib/ubiquibot"] |
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.
Smart way to share types!
Here is the latest branch that is currently powering the netlify production deployment. https://github.com/pavlovcik/ubiquibot/tree/refactor/move-to-delegated-compute P.S. another reason why we are redoing the architecture is to be compatible with the Cloudflare Worker runtime. Sorry for not clearly telling you about this before you dove into the development branch here! |
@pavlovcik all good, I am myself not very familiar yet with the whole architecture so I serve as monkey tester for now. I'll move to the other branch and see what's going on, and update the PR accordingly. |
@@ -131,10 +49,10 @@ export const parseYAML = async (data: any): Promise<any | undefined> => { | |||
} | |||
}; | |||
|
|||
export const parseJSON = async (data: any): Promise<any | undefined> => { | |||
export const parseJSON = async <T>(data: 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.
Seems like a wise way to set up types.
I'll have @rndquu handle the real review. |
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.
It seems that the url is pointing to the conf file that moved, I will take a look at it tomorrow. Also since it's completely different from what @pavlovcik might have to change the whole system altogether 😞 |
So after looking into it, I think this should be broke down in more steps. First we should use the old ubiquibot conf to solve this issue, which was to remove all the |
d002b0a
to
baf6f8d
Compare
conf = JSON.parse(conf); | ||
defaultConf = conf as IConf; | ||
defaultConf["private-key-encrypted"] = ""; | ||
defaultConf[PRIVATE_ENCRYPTED_KEY_NAME] = ""; |
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 am pretty sure that we changed our strategy to be either null
or the correct string for these situations.
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.
Changed it to undefined
(type is string | undefined
with the current conf from Ubiqubot repo)
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 prefer null
as it is unambiguously empty vs undefined
which could be a mistake. Why did you choose undefined
?
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 didn't, it comes from the types in ubiquibot-config-default.ts
I think that you should be based off of my branch and then work off of my fork instead actually. This code is very much out of date. We also eventually need to sync it back over to this repository. Perhaps you can help with this to avoid this confusion in the future with new contributors. We also generally do not allow force pushes (accidentally left it on for this repository apparently) |
@pavlovcik sure I would like to update the code but it would require refractor on both repositories which I thought would be out of scope for this ticket specifically. The other thread we have about how to change the configuration to be more easily sharable between projects should solve that issue later on. |
Please feel free to file the issues and you can receive credit for writing those issue specifications assuming those tasks get funded! In this way, if somebody else has extra bandwidth, they might be able to help you proceed in parallel. |
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.
The build is failing, could you fix it?
I added the submodule checkout in the build steps, I don't know how they were checked out before. |
} catch (error) { | ||
return undefined; | ||
} | ||
}; | ||
|
||
export const YAMLStringify = (value: any) => YAML.stringify(value, { defaultKeyType: "PLAIN", defaultStringType: "QUOTE_DOUBLE", lineWidth: 0 }); | ||
|
||
export const getConf = async (initial: boolean = false): Promise<string | undefined> => { | ||
export const getConf = async (): Promise<string | undefined> => { |
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 guess I need to update the lint settings after this is merged. I banned these arrow functions in favor of named functions (unless they are in a callback.)
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.
Just have one hangup related to the X25519 public key.
For context, this allows us to add the funding wallet's private key inside of the public .yml file, while only letting the bot decrypt it (we have the X25519 private key in our secrets.)
In other news, given that we figured out how to authenticate requests as the bot, wouldn't it make sense for us to read the raw key directly from their organization/repository secrets? Then we no longer need to include sensitive data inside of these public yml
files. https://chat.openai.com/share/ba4e137d-4882-4c7a-b23a-2fcb541c83fb
They were not. Since https://github.com/foundry-rs/forge-std and https://github.com/Uniswap/permit2 were not used in the frontend build process (only in solidity tests) there were no errors.
I am not really aware of the latest bot'a architecture, but, yes, if we pass wallet private key to some script in the github workflow file then we can stop saving encrypted evm private keys in |
Is there any action I should take about this? |
Can you make it based off of my fork and latest branch for now? We can handle merging mine in to the ubiquibot repository later |
@pavlovcik Yes I can, but then this PR should just be deleted because everything should be changed to match your fork / branch I believe |
That was my concern I voiced several days ago. Providing updates based on severely outdated code will not do us much good! It may be best for you to focus efforts on the kernel instead if you want to work on the bot itself! @rndquu rfc |
@pavlovcik Yep you're right, I just thought we could do it step by step so we can merge this then I work on the config sharing from your branch, then later merge everything together, because it seems critical to have the code base updated on the main repo |
+1 The PR looks good, so I would merge it and let @FernandVEYRIER proceed with "merge this then I work on the config sharing from your branch, then later merge everything together" |
Thanks @rndquu. Currently working on the bot npm package config, which should hopefully fix this problem later on by simply adding the package! |
@FernandVEYRIER Currently debugging a build error so that we can use the new continuous deployment I worked on all night: m1:pay.ubq.fi nv$ git submodule update --init --recursive
fatal: transport 'file' not allowed
fatal: Fetched in submodule path 'lib/ubiquibot', but it did not contain 8afd7cfa13e9731fb49b5f5a03f605258458d649. Direct fetching of that commit failed.
m1:pay.ubq.fi nv$ Any ideas? I'm here: https://github.com/ubiquity/pay.ubq.fi/actions/runs/7966008208/job/21746567166 Update: Those files don't exist anymore! |
Resolves #135