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.

Sign up for GitHub

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

Jump to bottom

feat(chart): add auctioneer chart and just commands #1738

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

feat(chart): add auctioneer chart and just commands #1738

wants to merge 6 commits into from

Conversation

itamarreif
Copy link
Contributor

@itamarreif itamarreif commented Oct 24, 2024 •
edited
Loading

Summary

Adds a Helm chart for deploying the Auctioneer.

Background

PR #1556 adds the auctioneer binary. This binary has clients to both rollup and sequencer nodes.

Changes

  • Add the charts/auctioneer directory for the auctioneer binary
  • Add commands to charts/deploy.just for deploying/deleting the auctioneer chart in our k8s cluster
  • Add dev/auctioneer/values.yaml for dev values for the auctioneer.
  • Add the auctioneer's dev wallet to the genesis allocations in dev/validators/all.yaml so that the auctioneer has a balance while testing against the dev cluster.
  • Expose the bundle grpc service from the rollup chart so that the auctioneer can talk to it from within the cluster

Testing

How are these changes tested?

Breaking Changelist

  • Bulleted list of breaking changes, any notes on migration. Delete section if none.

Related Issues

Link any issues that are related, prefer full github links.

closes #1766

@github-actions github-actions bot added the cd label Oct 24, 2024
@itamarreif itamarreif self-assigned this Oct 28, 2024
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/chart branch 3 times, most recently from 15c6845 to c77e0c0 Compare October 28, 2024 23:21
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/chart branch 2 times, most recently from ee7a54b to 2bb00cf Compare October 29, 2024 18:06
@itamarreif
Copy link
Contributor Author

Noting that I should change this to allow the auctioneer to connect to the sequencer and evm-rollup using UDS instead of TCP.

I still need to make that change in the auctioneer's binary (pr #1556), so haven't updated this yet. Tracking this in issue #1777.

@itamarreif itamarreif force-pushed the itamarreif/auctioneer/eng-824 branch 2 times, most recently from 32eb3bc to 45810bf Compare November 4, 2024 22:49
quasystaty1
quasystaty1 reviewed Nov 6, 2024
View reviewed changes
Copy link
Contributor

@quasystaty1 quasystaty1 left a comment

Choose a reason for hiding this comment

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

Some config suggestions and small fixes. also, should consider make all name and labels configurable for multiple auctioneer deployments, but assuming there will be only one auctioneer running this is less of an issue.

charts/deploy.just Outdated Show resolved Hide resolved
charts/evm-rollup/templates/service.yaml Outdated Show resolved Hide resolved
dev/values/auctioneer/values.yaml Outdated Show resolved Hide resolved
dev/values/validators/all.yml
@@ -36,6 +36,9 @@ genesis:
# address of the bridge account. needs funds to sign bridge init tx
- address: astria1d7zjjljc0dsmxa545xkpwxym86g8uvvwhtezcr
balance: "69000000"
# address of the auctioneer account. needs funds to submit auction results.
- address: astria1l5hg09ak99cfua5gu33j2m2se8eunv8nazyxtr
balance: "69000420"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should make sure dev private key is known, preferably by adding to default dev/values/auctioneer/values.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's in dev/values/auctioneer/values.yaml here:

astria/dev/values/auctioneer/values.yaml

Line 12 in 9aaf752

sequencerPrivateKey:

@github-actions github-actions bot added documentation Improvements or additions to documentation sequencer-relayer pertaining to the astria-sequencer-relayer crate labels Nov 6, 2024
@itamarreif itamarreif changed the base branch from itamarreif/auctioneer/eng-824 to main November 6, 2024 18:09
@github-actions github-actions bot added the ci issues that are related to ci and github workflows label Nov 6, 2024
joroshiba
joroshiba reviewed Nov 20, 2024
View reviewed changes
Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

LGTM, nice base chart. couple very small nits.

charts/auctioneer/values.yaml
rollupGrpcEndpoint: ""
rollupId: ""
latencyMarginMs: ""
logLevel: "info"
Copy link
Member

Choose a reason for hiding this comment

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

We have generally set the logLevel in most through global I believe although I'm open to that not being ideal and doing it here.

charts/auctioneer/values.yaml
sequencerAddressPrefix: astria
rollupGrpcEndpoint: ""
rollupId: ""
latencyMarginMs: ""
Copy link
Member

Choose a reason for hiding this comment

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

This might be worth providing a default example for, since I'm not sure whether it should be 1 or 2000

charts/evm-rollup/templates/service.yaml
@@ -13,6 +13,10 @@ spec:
- name: ws-rpc-svc
port: {{ .Values.ports.wsRPC }}
targetPort: ws-rpc
- name: execution-grpc-svc
# TODO: this service should be served over UDS
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove comment

.github/workflows/docker-build.yml
@@ -12,6 +12,7 @@ on:
required: false
type: choice
options:
- auctioneer
Copy link
Member

Choose a reason for hiding this comment

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

should probably be in the docker pr vs here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quasystaty1 quasystaty1 quasystaty1 left review comments

@joroshiba joroshiba joroshiba left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@itamarreif itamarreif

Labels
auctioneer cd ci issues that are related to ci and github workflows documentation Improvements or additions to documentation sequencer-relayer pertaining to the astria-sequencer-relayer crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Auctioneer chart
4 participants
@itamarreif @joroshiba @quasystaty1 @SuperFluffy