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

deposit: refactor to forward deploys to L1 #94

Merged
merged 22 commits into from
Jun 6, 2024

Conversation

marijanp
Copy link
Contributor

@marijanp marijanp commented May 7, 2024

Motivation

  • Checking signatures and adding the deposit to the batch is not required to be done in the deposit endpoint.
  • It should rather according to casper best-practices forward deploys to the L1.
  • This deposit-deploy will be prepared in the kairos-cli and signed
  • The signed deploy is sent to the L1 through kairos
  • Once the L1 processes the deploy, an event will be triggered.
  • The kairos server will then add the deposit to the batch.

TODOS

    1. Don't break the other server tests, i.e. implement alternative deposit handler which can be used for property tests and only adds deposits to the batch
    1. Integrate the deposit contract i.e. wait for it to be merged.
    1. Implement the kairos-cli counterpart preparing the deposit deploy
    1. Update the end-to-end-test

Successive PR

@marijanp marijanp changed the base branch from main to implement-deposit-cli-3 May 7, 2024 20:15
@marijanp marijanp self-assigned this May 7, 2024
@marijanp marijanp added the demo Required for our first demo label May 7, 2024
@jonas089
Copy link
Contributor

jonas089 commented May 7, 2024

"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
Copy link
Contributor

jonas089 commented May 8, 2024

"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
Copy link
Contributor Author

marijanp commented May 8, 2024

@jonas089 the current implementation of the server suggests something different

pub async fn deposit_handler(
_: DepositPath,
state: State<Arc<BatchStateManager>>,
Json(body): Json<PayloadBody>,
) -> Result<(), AppErr> {
tracing::info!("parsing transaction data");
let signing_payload: SigningPayload =
body.payload.as_slice().try_into().context("payload err")?;
let deposit = match signing_payload.body {
TransactionBody::Deposit(deposit) => deposit.try_into().context("decoding deposit")?,
_ => {
return Err(AppErr::set_status(
anyhow!("invalid transaction type"),
StatusCode::BAD_REQUEST,
))
}
};
let public_key = body.public_key;
let nonce = signing_payload.nonce.try_into().context("decoding nonce")?;
state
.enqueue_transaction(Signed {
public_key,
nonce,
transaction: Transaction::Deposit(deposit),
})
.await
}

@jonas089
Copy link
Contributor

jonas089 commented May 8, 2024

@jonas089 the current implementation of the server suggests something different

pub async fn deposit_handler(
_: DepositPath,
state: State<Arc<BatchStateManager>>,
Json(body): Json<PayloadBody>,
) -> Result<(), AppErr> {
tracing::info!("parsing transaction data");
let signing_payload: SigningPayload =
body.payload.as_slice().try_into().context("payload err")?;
let deposit = match signing_payload.body {
TransactionBody::Deposit(deposit) => deposit.try_into().context("decoding deposit")?,
_ => {
return Err(AppErr::set_status(
anyhow!("invalid transaction type"),
StatusCode::BAD_REQUEST,
))
}
};
let public_key = body.public_key;
let nonce = signing_payload.nonce.try_into().context("decoding nonce")?;
state
.enqueue_transaction(Signed {
public_key,
nonce,
transaction: Transaction::Deposit(deposit),
})
.await
}

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
Copy link
Contributor Author

marijanp commented May 8, 2024

@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
Copy link
Contributor

jonas089 commented May 8, 2024

@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.

@marijanp marijanp changed the title deposit refactor to forward deploys to L1 deposit: refactor to forward deploys to L1 May 8, 2024
Base automatically changed from implement-deposit-cli-3 to main May 9, 2024 14:57
@marijanp marijanp marked this pull request as ready for review May 22, 2024 12:42
Copy link

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

Minimum allowed coverage is 60%

Generated by 🐒 cobertura-action against 8a0736a

Copy link
Contributor

@Avi-D-coder Avi-D-coder left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jonas089 jonas089 left a comment

Choose a reason for hiding this comment

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

lgtm

@Avi-D-coder Avi-D-coder merged commit 535790a into main Jun 6, 2024
4 checks passed
@Avi-D-coder Avi-D-coder deleted the implement-deposit-cli-4 branch June 6, 2024 22:02
@koxu1996
Copy link
Contributor

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
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

4 participants