cspr-rad / kairos

Apache License 2.0
2 stars 0 forks source link

deposit: refactor to forward deploys to L1 #94

Closed marijanp closed 3 months ago

marijanp commented 4 months ago

Motivation

TODOS

Successive PR

jonas089 commented 4 months ago

"Checking signatures and adding the deposit to the batch is not required to be done in the deposit endpoint."

I'm not sure what is meant by this?

The Kairos server/ node will query the L1 for deposits and then roll them up in a batch together with Transactions and Withdrawals and produce a proof.

The proof is then submitted to another entry point of the demo smart contract that will verify it, extract the new root from the journal and update the contract state to store that new root.

jonas089 commented 4 months ago

"It should rather according to casper best-practices forward deploys to the L1."

But native L1 deposits will still occur, so not every deposit will be forwarded from the L2 to the L1?

If you want to route Deposits through the L2 then yes this is the way to do it, but that's an additional means of depositing and not a replacement?

marijanp commented 4 months ago

@jonas089 the current implementation of the server suggests something different https://github.com/cspr-rad/kairos/blob/27ef3fc980eef4e099a06f192cd68c14729ba3a9/kairos-server/src/routes/deposit.rs#L24-L52

jonas089 commented 4 months ago

@jonas089 the current implementation of the server suggests something different https://github.com/cspr-rad/kairos/blob/27ef3fc980eef4e099a06f192cd68c14729ba3a9/kairos-server/src/routes/deposit.rs#L24-L52

But if you forward a signed deploy to the L2 you might as well use the CLI to forward the deploy directly to the L1.

It will be recorded as an Event and then picked up by the L2.

Handling it differently introduces unwanted complexity.

For Withdrawals we will have to route the Deploy through the L2 but I'm not sure about deposits... Native L1 deposits and a good CLI should be good enough?

Another edit: I think that for Withdrawals it's actually best for the L2 to verify a signature and then create and dispatch the deploy.

marijanp commented 4 months ago

@jonas089 If you ask me I would get rid of the deposit endpoint entirely. However the casper docs here and here suggest that we should forward through a backend. Moreover @Avi-D-coder is interested in having an endpoint to be able to do low-setup property-tests.

jonas089 commented 4 months ago

@jonas089 If you ask me I would get rid of the deposit endpoint entirely. However the casper docs suggests that we should forward through a backend. Moreover @Avi-D-coder is interested in having an endpoint to be able to do low-setup property-tests.

Okay, shouldn't make a big difference, it'll work either way.

github-actions[bot] commented 3 months ago

File Coverage
All files 59% :x:
kairos-tx/src/asn.rs 51% :x:
kairos-tx/src/error.rs 0% :x:
kairos-crypto/src/implementations/casper.rs 6% :x:
kairos-server/src/routes/deposit.rs 72% :white_check_mark:
kairos-server/src/routes/deposit_mock.rs 88% :white_check_mark:
kairos-server/src/routes/transfer.rs 90% :white_check_mark:
kairos-server/tests/transactions.rs 90% :white_check_mark:
demo-contract-tests/tests/test_fixture/mod.rs 97% :white_check_mark:
demo-contract-tests/tests/test_fixture/wasm_helper.rs 66% :white_check_mark:
kairos-server/src/config.rs 0% :x:
kairos-server/src/errors.rs 10% :x:
kairos-server/src/lib.rs 95% :white_check_mark:
kairos-server/src/state.rs 90% :white_check_mark:
kairos-server/src/utils.rs 22% :x:
kairos-test-utils/src/cctl.rs 90% :white_check_mark:
kairos-server/src/state/transactions/batch_state.rs 36% :x:
kairos-server/src/state/transactions.rs 66% :white_check_mark:
kairos-server/src/state/trie.rs 35% :x:
kairos-test-utils/src/cctl/parsers.rs 88% :white_check_mark:

Minimum allowed coverage is 60%

Generated by :monkey: cobertura-action against 8a0736a3fab68fb9fca5fd1e7ac56ce66c6602b6

koxu1996 commented 3 months ago

I would avoid compile-time environment variables, that breaks compilation outside of Nix. It's also annoying to see analyzer errors in IDE:

image

marijanp commented 3 months ago

So if you don't have this environment variable sourced in your development environment and run the code it will instead throw a runtime exception. Also sourcing environment variables is not accomplished with Nix but rather captured with Nix. My local development environment sources these environment variables for me automatically when I enter the kairos repository.