Skip to content

refactor(core, proto)!: define bridge memos in proto #1285

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

Merged
SuperFluffy merged 8 commits into from
Jul 25, 2024
Merged

refactor(core, proto)!: define bridge memos in proto #1285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0c5be83
refactor(core, proto)!: define bridge memos in proto
SuperFluffy Jul 22, 2024
8921832
rework tests to use astria-cli-rs instead of astria-cli-go
SuperFluffy Jul 25, 2024
a6495a8
update github workflow
SuperFluffy Jul 25, 2024
8927a93
remove no longer necessary charts
SuperFluffy Jul 25, 2024
a3ee474
add back snapshot tests for proto defined json memos
SuperFluffy Jul 25, 2024
b55fa71
add comments on the memo protos
SuperFluffy Jul 25, 2024
b83d6c5
clarify error message for unset deposit address
SuperFluffy Jul 25, 2024
c3d86d0
fix comment explaining mult factor
SuperFluffy Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 8 additions & 21 deletions .github/workflows/docker-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ jobs:
uses: helm/kind-action@v1
with:
install_only: true
- name: Install astria cli (rust)
run: just install-cli
- name: Fetch and install celestia-appd
run: just get-celestia-appd v1.9.0 Linux x86_64
Copy link
Copy Markdown
Contributor Author

@SuperFluffy SuperFluffy Jul 25, 2024

Choose a reason for hiding this comment

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

This could be avoided if we make celestia-local into a stateful set. We could then kubectl execute from inside the celestia-local container, making this step unnecessary.

Copy link
Copy Markdown
Member

@joroshiba joroshiba Jul 25, 2024

Choose a reason for hiding this comment

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

I'll note that celestia-local being a statefulset is technically correct and I believe basically a oneliner + file rename for consistency.

Some concern that this creates room for key collision running locally. I think we can refactor the smoke-test in general after merge though.

Copy link
Copy Markdown
Contributor Author

@SuperFluffy SuperFluffy Jul 25, 2024

Choose a reason for hiding this comment

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

Yes, this indeed has that issue, which running directly inside a stateful set wouldn't have. Followup sounds good. I will create an issue.

- name: Log in to GHCR
uses: docker/login-action@v2
with:
Expand All @@ -204,27 +208,10 @@ jobs:
just deploy cluster
kubectl create secret generic regcred --from-file=.dockerconfigjson=$HOME/.docker/config.json --type=kubernetes.io/dockerconfigjson
echo -e "\n\nDeploying with astria images tagged $TAG"
just deploy ibc-test-infra $TAG
just build-and-load-bridge-tester-image $TAG
- name: Run IBC Bridge Tester
timeout-minutes: 2
run: |
TAG=sha-$(git rev-parse --short HEAD)
printlogs() {
echo "IBC Transfer Test failed, printing logs..."
kubectl describe job bridge-tester-chart -n astria-dev-cluster
kubectl logs job/bridge-tester-chart --all-containers -n astria-dev-cluster
exit 1
}
just deploy bridge-tester $TAG
# timeout before the gh job times out so we can print logs
kubectl wait --for=condition=complete --timeout=90s job/bridge-tester-chart -n astria-dev-cluster || printlogs
JOB_STATUS=$(kubectl get job bridge-tester-chart -n astria-dev-cluster -o jsonpath='{.status.succeeded}')
if [ "$JOB_STATUS" != "1" ]; then
printlogs
else
echo "IBC Transfer Test passed"
fi
just ibc-test deploy $TAG
- name: Run IBC ICS20 Transfer test
timeout-minutes: 3
run: just ibc-test run ./celestia ./celestia-appd
Copy link
Copy Markdown
Contributor Author

@SuperFluffy SuperFluffy Jul 25, 2024

Choose a reason for hiding this comment

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

Similar to above: the arguments are required because we need to provide a celestia "home" directory, and the path to the celestia-appd executable. It would be neater if we could run straight from the container running celestia-appd.


docker:
if: ${{ always() && !cancelled() }}
Expand Down
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 0 additions & 23 deletions charts/bridge-test/.helmignore
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions charts/bridge-test/Chart.yaml
View file
Open in desktop

This file was deleted.

58 changes: 0 additions & 58 deletions charts/bridge-test/files/scripts/test-ibc-transfer.sh
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions charts/bridge-test/templates/_helpers.tpl
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions charts/bridge-test/templates/configmap.yaml
View file
Open in desktop

This file was deleted.

41 changes: 0 additions & 41 deletions charts/bridge-test/templates/job.yaml
View file
Open in desktop

This file was deleted.

38 changes: 0 additions & 38 deletions charts/bridge-test/values.yaml
View file
Open in desktop

This file was deleted.

12 changes: 12 additions & 0 deletions charts/deploy.just
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
mod ibc-test

##############################################
## Deploying and Running using Helm and K8s ##
##############################################
Expand Down Expand Up @@ -225,6 +227,16 @@ init-rollup-bridge rollupName=defaultRollupName evmDestinationAddress=evm_destin
--fee-asset=$FEE_ASSET --asset=$ASSET || exit 1


init-ibc-bridge privateKey asset feeAsset rollupName=defaultRollupName:
astria-cli sequencer init-bridge-account \
--rollup-name {{ rollupName }} \
--private-key {{ privateKey }} \
--sequencer.chain-id {{ sequencer_chain_id }} \
--sequencer-url {{ sequencer_rpc_url }} \
--fee-asset {{ feeAsset }} \
--asset {{ asset }}
Comment on lines +230 to +237
Copy link
Copy Markdown
Member

@joroshiba joroshiba Jul 25, 2024

Choose a reason for hiding this comment

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

nit: duplication with above init-rollup-bridge code. this can be follow up cleanup though.

Copy link
Copy Markdown
Contributor Author

@SuperFluffy SuperFluffy Jul 25, 2024

Choose a reason for hiding this comment

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

It's a partial duplication. feeAsset and asset are hardcoded in init-rollup-bridge, and there is an additional lock command to lock to the rollup. Could be refactored.



eth_rpc_url := "http://executor.astria.localdev.me/"
eth_ws_url := "ws://ws-executor.astria.localdev.me/"
bridge_tx_bytes := "0xf8f280843c54e7f182898594a58639fb5458e65e4fa917ff951c390292c24a15880de0b6b3a7640000b884bab916d00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002d617374726961313777306164656736346b7930646178776432756779756e65656c6c6d6a676e786c333935303400000000000000000000000000000000000000820a96a086b85348c9816f6d34533669db3d3626cf55eecea6a380d4d072efb1839df443a04b8b60c8b91dd30add1ca4a96097238d73bab29b0a958322d9a51755d5a5f287"
Expand Down
Loading