Skip to content

refactor: sync repo with kwil v0.7#117

Merged
MicBun merged 20 commits into
mainfrom
refactor/upgrade-zero-seven
Mar 28, 2024
Merged

refactor: sync repo with kwil v0.7#117
MicBun merged 20 commits into
mainfrom
refactor/upgrade-zero-seven

Conversation

@MicBun
Copy link
Copy Markdown
Contributor

@MicBun MicBun commented Mar 14, 2024

Description

Complete Refactor to sync with Kwil v0.7

Related Issue

#114

Motivation and Context

One step closer for this goal: #114

How Has This Been Tested?

  1. Run this following task command
task build
task postgres
task kwild
  1. Seed the data using setup script on branch main
  2. Test by calling action kwil-cli utils ping and some actions
kwil-cli database call -a=get_index date: date_to: -n=cpi

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update (including inline docs)
  • Other (please describe):

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Checklist Explanation:

How to Review this PR:

Additional Information:

Managed to run kwild --autogen and call get index and value with the default date, need to take a look in separate PR, if this architecture is acceptable please make future PR to this branch instead of main, after we finalize everything then we may merge to main.

image
image

  • Problem: Rewrite CI to new architecture
  • Problem: Rewrite Go Test
  • Problem: Date Range Not Working
  • Problem: Rewrite README.md section
  • Problem: Migrate Setup Scripts to the new architecture
  • Problem: Migrate CDK to the new architecture
  • Problem: Rewrite Dockerfile to the new architecture

@MicBun MicBun self-assigned this Mar 14, 2024
@MicBun MicBun linked an issue Mar 14, 2024 that may be closed by this pull request
9 tasks
@MicBun MicBun force-pushed the refactor/upgrade-zero-seven branch from 6be6013 to ce92cb4 Compare March 15, 2024 08:05
@MicBun MicBun force-pushed the refactor/upgrade-zero-seven branch from ce92cb4 to 8e42c80 Compare March 15, 2024 08:45
@MicBun MicBun added type: docs Improvements or additions to documentation type: feat New feature or request DO NOT MERGE labels Mar 15, 2024
@MicBun MicBun requested a review from zolotokrylin March 15, 2024 08:54
@MicBun MicBun marked this pull request as ready for review March 15, 2024 09:08
@zolotokrylin zolotokrylin enabled auto-merge (squash) March 15, 2024 09:14
@zolotokrylin
Copy link
Copy Markdown
Member

@MicBun please resolve conflicts

@zolotokrylin zolotokrylin disabled auto-merge March 15, 2024 09:14
@MicBun MicBun force-pushed the refactor/upgrade-zero-seven branch from 44caa28 to b012f45 Compare March 15, 2024 09:22
@outerlook
Copy link
Copy Markdown
Contributor

outerlook commented Mar 15, 2024

Hey @MicBun, so the idea is merging all the problems as PRs to this branch and finally merge it, right?

If you want some help on some of these, maybe we can create sub-issues where we can control who's taking what.
Unless you think if I continue to work on gateway, it won't create too much conflict for you to keep tackling this issue.

@MicBun
Copy link
Copy Markdown
Contributor Author

MicBun commented Mar 15, 2024

Hey @MicBun, so the idea is merging all the problems as PRs to this branch and finally merge it, right?

@outerlook
Yes, that's the idea.

If you want some help on some of these, maybe we can create sub-issues where we can control who's taking what. Unless you think if I continue to work on gateway, it won't create too much conflict for you to keep tackling this issue.

Sure, you may continue to do so, please keep in mind some functions is missing in the new kwil 0.7 like Dataset interface. Since this one is a big change (980+ files affected), maybe should wait for other responses

@outerlook outerlook mentioned this pull request Mar 15, 2024
9 tasks
@zolotokrylin
Copy link
Copy Markdown
Member

@MicBun @outerlook let's follow Michael's suggestion 👍

…/upgrade-zero-seven

# Conflicts:
#	.github/workflows/ci.yaml
#	scripts/ci-tests.sh
#	scripts/use_base_schema.sh
#	truflation/base_schema/base_schema.kf
#	truflation/develop_experiments/develop_scripts.md
#	truflation/docker/tsn.dockerfile
#	truflation/infra/cdk_main.go
@outerlook
Copy link
Copy Markdown
Contributor

For now, I pushed directly some commits to eliminate upfront conflicts, and now, I'll open PRs to it

outerlook
outerlook previously approved these changes Mar 20, 2024
@MicBun
Copy link
Copy Markdown
Contributor Author

MicBun commented Mar 20, 2024

Hi @zolotokrylin, the merge is ready now, you might want to have a final look.

outerlook
outerlook previously approved these changes Mar 20, 2024
@MicBun MicBun removed the type: docs Improvements or additions to documentation label Mar 21, 2024
@outerlook outerlook linked an issue Mar 21, 2024 that may be closed by this pull request
outerlook
outerlook previously approved these changes Mar 22, 2024
@zolotokrylin
Copy link
Copy Markdown
Member

Well done guys! Great work!
Do I need to set up the Secretes before merging? (ref to this comment #117 (comment))

@outerlook
Copy link
Copy Markdown
Contributor

outerlook commented Mar 22, 2024

@zolotokrylin

Do I need to set up the Secretes before merging?

That would be good. It's necessary for the new deployment action to work correctly. Note that whitelist_wallets is a variable and not a secret, but PRIVATE_KEY is a secret. However they are accessible by who has access to SSH the EC2 instance.

outerlook
outerlook previously approved these changes Mar 23, 2024
@markholdex
Copy link
Copy Markdown
Collaborator

Hey @MicBun and @outerlook are we waiting here for @zolotokrylin to setup the wallets and private keys?

@MicBun
Copy link
Copy Markdown
Contributor Author

MicBun commented Mar 25, 2024

to setup the wallets and private keys?

@markholdex Yes, that would be good. otherwise, it is only going to be merged with main, while staging will be crashed due to missing private keys.

@zolotokrylin
Copy link
Copy Markdown
Member

Note that whitelist_wallets is a variable and not a secret, but PRIVATE_KEY is a secret. However, they are accessible by those who have access to SSH the EC2 instance.

@outerlook does it mean that our "node operator" users will have access to PRIVAT_KEY? Or will they have their own private keys? How will it work when we need to deploy other nodes run by third parties?

@outerlook
Copy link
Copy Markdown
Contributor

outerlook commented Mar 25, 2024

@zolotokrylin

does it mean that our "node operator" users will have access to PRIVAT_KEY? Or will they have their own private keys? How will it work when we need to deploy other nodes run by third parties?

PRIVATE_KEY defined here is for the configured wallet that will be creating the databases, and also pushing data to TSN. He will be the one permissioned to push data to the schemas.

If third parties have their schemas, they should have their own PRIVATE_KEY. Then, the owner property for their schemas will correctly be different.

It will be accessible because it is currently available as a plaintext file at our EC2 instance. In the future, for further protection, we could also encrypt the files, but that could happen later, right?

whitelist_wallets controls who has read access to data. Say our frontend (serverside) needs to access that data and owns a wallet whose public key is 0x123...321. We whitelist that wallet on this property. If only the owner must have access to the reading, it could be empty.

For now, of course, you can set a public value for these and share it with us (e.g.: 000...01 But as soon as you think this should be protected, it should be shared secretly if at all)

* deployments/infra/cdk_main.go: make kwil gateway binary available in ec2 instance

This PR extracts logic for creating ec2 role into its own function. Also it adds a boot up script to ec2 instance to fetch from s3 the kwil gateway binary.

Resolves #109.

* apply pr comments

keep binary out of tmp folder

* address pr comments

change place of newName variable

* address pr comment

move binary to /usr/local/bin

* address pr comments

rewrite kwilGatewayBinaryScript
@zolotokrylin
Copy link
Copy Markdown
Member

@outerlook thank you!

  1. can we dynamically update the whitelisted addresses?
  2. about the PRIVATE KEY – noted, thanks!

@outerlook
Copy link
Copy Markdown
Contributor

@zolotokrylin

can we dynamically update the whitelisted addresses

Not as it is now :(
Dynamic whitelist table would be the 4th here

However, the possible next iteration, with a metadata table, already foresees a dynamic behavior here

@MicBun MicBun merged commit fb580b8 into main Mar 28, 2024
@MicBun MicBun deleted the refactor/upgrade-zero-seven branch March 28, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feat New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Goal: Upgrade to v0.7 Problem: Query errors from CLI are opaque

5 participants