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

Onboarding updates #135

Closed
rndquu opened this issue Sep 14, 2023 · 20 comments · Fixed by #154
Closed

Onboarding updates #135

rndquu opened this issue Sep 14, 2023 · 20 comments · Fixed by #154

Comments

@rndquu
Copy link
Member

rndquu commented Sep 14, 2023

Depends on ubiquity/ubiquibot#725

There is the onboarding page where partners can generate a default bot's config (example) for their organizations.

It requires a few fixes:

  1. When Refactor default bot config from json to ts ubiquibot#725 is implemented we should refactor the onboarding code to work with the bot's .ts default config instead of .json
  2. Original comment. Remove all any type declarations in the onboarding page
  3. Original comment. Remove config interface and make it dynamically typed based on the default bot's config (there should be a solution that "extracts" type from a js object). Simply put, config typings must be supported but without declaring them explicitly.
@gentlementlegen
Copy link
Member

/start

Copy link

ubiquibot bot commented Feb 12, 2024

Warning! This task was created over 151 days ago. Please confirm that this issue specification is accurate before starting.
DeadlineMon, Feb 12, 12:12 PM UTC
Registered Wallet 0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED
Tips:
  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

@gentlementlegen
Copy link
Member

@rndquu After looking into it, I think there would be different approaches that we should consider, all having their pros and cons.

Generating script for the configuration

It would work similarly to the script that currently pulls the configuration from the repo, e.g. downloading the file from the Ubiquity Bot directly.
Pros

  • easy to setup

Cons

  • Need to run the script on builds and locally to have always the latest version
  • Doesn't handle deps (if file A includes file B)
  • No knowledge of the version pulled, disconnected from versioning

Git Submodule

Pros

  • linked with versioning
  • knows about deps

Cons

  • pulls the whole repo (can be fixed by having a separate branch only containing the needed files, but then would require more setup on that repo as well)
  • need to update the version manually (could be fixed targeting HEAD)

Git Subtree

Pros

  • seamlessly adds the other codebase
  • uses version control

Cons

  • not easy to spot that the files are from a tree, need to be documented
  • it's also complicated to split only the needed files

It seems to me that having a submodule would be the best choice. Now I could try to do a partial clone to avoid having the whole other project in it. Please share your thoughts if any other approach sounds better!

@gentlementlegen
Copy link
Member

Follow up question, there are inside the onboarding.ts file some configuration values that are not present in the configuration structure:

  • "safe-address"
  • "private-key-encrypted"

Should they be added in the bot config, or are specific for the onboarding?

@rndquu
Copy link
Member Author

rndquu commented Feb 12, 2024

@FernandVEYRIER

After looking into it, I think there would be different approaches that we should consider, all having their pros and cons.

Using git submodule is fine

@rndquu
Copy link
Member Author

rndquu commented Feb 12, 2024

Follow up question, there are inside the onboarding.ts file some configuration values that are not present in the configuration structure:

  • "safe-address"
  • "private-key-encrypted"

Should they be added in the bot config, or are specific for the onboarding?

I'm not deeply aware of the latest bot's architecture. @wannacfuture @whilefoo Do we still need to set safe-address and private-key-encrypted in the bot's config? Chances are that the current issue is not relevant anymore.

@0x4007
Copy link
Member

0x4007 commented Feb 13, 2024

We migrated from kebab case to camel case. Kebab case properties should be deprecated.

safe-address definitely no
But we are using a value called evmPrivateKeyEncrypted which probably corresponds with the other.

@gentlementlegen
Copy link
Member

We migrated from kebab case to camel case. Kebab case properties should be deprecated.

safe-address definitely no But we are using a value called evmPrivateKeyEncrypted which probably corresponds with the other.

Thank you for the info @pavlovcik. Then, I will change KEY_NAME to be evmPrivateKeyEncrypted, remove safe-address and use evmNetworkId. However with the current version in the development branch, evmPrivateKeyEncrypted is not present, but instead is called privateKeyEncrypted, if that is what you were referring to.

@rndquu
Copy link
Member Author

rndquu commented Feb 13, 2024

In our config the 3rd naming us used: evmPrivateEncrypted. Guys @wannacfuture @whilefoo could you confirm the correct naming (evmPrivateKeyEncrypted vs privateKeyEncrypted vs evmPrivateEncrypted)?

@0x4007
Copy link
Member

0x4007 commented Feb 13, 2024

Yes sorry I was not on my computer much of today but my mistake was not clarifying that the development branch on this repository is extremely outdated. As mentioned in our other thread @FernandVEYRIER the latest is here:

https://github.com/pavlovcik/ubiquibot/tree/refactor/move-to-delegated-compute

And again, this will be deprecated when we get the kernel stable. However we will port over the event handlers so it's not a wasted effort to continue working on this repository, as the "core" is quite stable. Just as long as you're doing work in the GitHub webhook event handlers instead of "core" logic which will be replaced by the new kernel.

@gentlementlegen
Copy link
Member

gentlementlegen commented Feb 14, 2024

@pavlovcik from the repo you sent, it seems that the default configuration is pulled from another repo according to a given context. It also depends on a lot of 3rd party libraries to generate the config itself. This complicates things a lot when it comes to integrate it in another repo. At that point I believe that having an endpoint serving the configuration would make more sense. I refer to the file https://github.com/pavlovcik/ubiquibot/blob/refactor/move-to-delegated-compute/src/utils/generate-configuration.ts since I need a default config.

Edit:
I think it might be fine with no default value as the script overrides it from the start by pulling a new config on top anyways, so we should be fine.

@0x4007
Copy link
Member

0x4007 commented Feb 14, 2024

Some other remarks is that I'm curious to see some type of caching approach because on Cloudflare Workers in our current state of research, it must fetch both the organization and repository configurations within the GitHub webhook event handler function.

This means that before it can respond to the user invoked event, the Worker needs to make a couple of fetches to those repositories and merge the configuration before it can respond.

The configuration is needed to modify the behavior of the Worker response for most situations I can think of so it seems like an optimization that will affect most use cases.

We also made a handler for pushes that validates the yml configurations which could be a good time to write to cache. Maybe we can write the cached configuration to Cloudflare KV storage or something.

@gentlementlegen
Copy link
Member

Another approach could be to locally generate the configuration during a build and store it as a file, thus not needing to poke the network at runtime anymore. It comes with drawbacks as well, such as the conf not being updated without a rebuild, and the need to embed it somewhere. The cache would speed things up but still depend on a network request I believe.

@0x4007
Copy link
Member

0x4007 commented Feb 14, 2024

locally generate the configuration during a build and store it as a file

Unfortunately the vision is to run a single kernel instance from a Cloudflare Worker that we control across all of our future partner organizations/repositories.

I didn't deeply do research here but I imagine that caching on ubiquibot-config.yml push updates and saving the merged configurations inside of Cloudflare infrastructure might save quite a bit on network roundtrip times. I assume that Cloudflare Workers can access Cloudflare infrastructure much faster than GitHub.

@gentlementlegen
Copy link
Member

I've been thinking about it for a while, but why not making it an npm package? That would solve the distribution issue, very easy to include in any project, with all the type and dependencies, we also get versioning so we don't break everything whenever we update?

@0x4007
Copy link
Member

0x4007 commented Feb 14, 2024

I'm not against distributing npm packages, but I guess it's not clear to me if you are implying that our partners can make other repositories and then run their own instances of the bot. But our business model mandates that our partners go to the GitHub marketplace and add our UbiquiBot to their organization/repositories as a GitHub App.

From here we can support Fiat payments (monthly fees or transaction fees) directly via GitHub and then middle-man/provide the liquidity between crypto or fiat (bank transfer or top up of our UbiquiCards) for the payouts to the contributors (this is our strong moat to establish a profit margin/service for charging transaction fees)

@gentlementlegen
Copy link
Member

I meant that the configuration could be it's own repository that we distribute as an npm package. This way any project that needs the configuration can just add something like "@ubiquibot/configuration" in the npm packages of the project, this way you get access to the configuration easily, with the parsing / generation methods as well.

@0x4007
Copy link
Member

0x4007 commented Feb 15, 2024

That's a great idea. I do envision a developer SDK for our plugins which absolutely should make configuration imports simple (as they seem to need to be handled by the plugin itself.)

Copy link

ubiquibot bot commented Feb 17, 2024

+ Evaluating results. Please wait...

Copy link

ubiquibot bot commented Feb 17, 2024

[ 80.9 WXDAI ]

@pavlovcik
Contributions Overview
ViewContributionCountReward
IssueComment655.8
ReviewComment625.1
Conversation Incentives
CommentFormattingRelevanceReward
We migrated from kebab case to camel case. Kebab case properties...
5.2
code:
  count: 2
  score: "2"
  words: 3
0.835.2
Yes sorry I was not on my computer much of today but my mistake ...
11.40.7811.4
Some other remarks is that I'm curious to see some type of cachi...
14.9
code:
  count: 1
  score: "1"
  words: 1
0.7814.9
> locally generate the configuration during a build and store it...
9.4
code:
  count: 2
  score: "2"
  words: 4
0.799.4
I'm not against distributing npm packages, but I guess it's not ...
11.60.711.6
That's a great idea. I do envision a developer SDK for our plugi...
3.30.853.3
Here is the latest branch that is currently powering the netlify...
5.90.285.9
I'll have @rndquu handle the real review. ...
0.80.230.8
I think that you should be based off of my branch and then work ...
7.20.577.2
> would require refractor on both repositories which I thought w...
4.10.464.1
> Is there any action I should take about this?

Can you make it...

2.50.522.5
That was my concern I voiced several days ago. Providing updates...
4.60.694.6

[ 91.2 WXDAI ]

@FernandVEYRIER
Contributions Overview
ViewContributionCountReward
IssueTask1.0050
IssueComment70
ReviewComment941.2
Conversation Incentives
CommentFormattingRelevanceReward
@rndquu After looking into it, I think there would be different ...
-
h2:
  count: 3
  score: "0"
  words: 9
li:
  count: 12
  score: "0"
  words: 115
0.84-
Follow up question, there are inside the `onboarding.ts` file so...
-
li:
  count: 2
  score: "0"
  words: 13
code:
  count: 3
  score: "0"
  words: 11
0.76-
> We migrated from kebab case to camel case. Kebab case properti...
-
code:
  count: 8
  score: "0"
  words: 10
0.84-
@pavlovcik from the repo you sent, it seems that the default con...
-0.75-
Another approach could be to locally generate the configuration ...
-0.76-
I've been thinking about it for a while, but why not making it a...
-0.74-
I meant that the configuration could be it's own repository that...
-0.84-
> I realized we didn't clearly document something that is crucia...
40.224
> Getting an error on start and the "Set" button doesn't do anyt...
3.80.433.8
So after looking into it, I think this should be broke down in m...
11.6
code:
  count: 3
  score: "3"
  words: 3
0.5211.6
@pavlovcik sure I would like to update the code but it would req...
8.40.58.4
> The [build](https://github.com/ubiquity/pay.ubq.fi/actions/run...
2.9
a:
  count: 1
  score: "1"
  words: 1
0.552.9
Is there any action I should take about this?...
0.90.460.9
@pavlovcik Yes I can, but then this PR should just be deleted be...
2.40.52.4
@pavlovcik Yep you're right, I just thought we could do it step ...
4.90.544.9
Thanks @rndquu. Currently working on the bot npm package config,...
2.30.712.3

[ 84 WXDAI ]

@rndquu
Contributions Overview
ViewContributionCountReward
IssueSpecification135.4
IssueComment323.8
ReviewComment224.8
Conversation Incentives
CommentFormattingRelevanceReward
Depends on https://github.com/ubiquity/ubiquibot/issues/725

T...

35.4

a:
  count: 7
  score: "7"
  words: 15
li:
  count: 3
  score: "3"
  words: 163
code:
  count: 3
  score: "3"
  words: 3
135.4
@FernandVEYRIER

After looking into it, I think there would...

1.20.651.2
> Follow up question, there are inside the onboarding.ts file ...
15

li:
  count: 2
  score: "2"
  words: 13
code:
  count: 5
  score: "5"
  words: 16
0.8115
In our [config](https://github.com/ubiquity/ubiquibot-config/blo...
7.6
a:
  count: 1
  score: "1"
  words: 1
code:
  count: 2
  score: "2"
  words: 6
0.727.6
> > The [build](https://github.com/ubiquity/pay.ubq.fi/actions/r...
18.4
a:
  count: 1
  score: "2"
  words: 1
code:
  count: 1
  score: "2"
  words: 1
0.3818.4
> I just thought we could do it step by step

+1

The PR lo...

6.40.736.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants