Deposit contract suggestions #95

marijanp · 2024-05-08T20:03:25Z

Incorporate refactorings after discussions with @Avi-D-coder.

Merge this branch into feature/deposit-contract then merge feature/deposit-contract into master.

Or directly merge this.

github-actions · 2024-05-08T20:18:46Z

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

Minimum allowed coverage is 60%

Generated by 🐒 cobertura-action against b663b89

kairos-cli: implement call to deposit endpoint and integration test

jonas089 · 2024-05-09T17:04:40Z

What's the idea behind this PR? Is it supposed to replace the other deposit contract PR?

.gitignore

kairos-contracts/demo-contract/contract/src/main.rs

Avi-D-coder · 2024-05-09T17:17:23Z

kairos-contracts/demo-contract/contract/src/utils/events.rs

+
+#[derive(Event)]
+pub struct Deposit {
+    pub account: Key,


What is this key type?
Can we use a more specific type?

The problem is that this could be an AccountHash or a ContractPackageHash.

If you follow how the code that creates the Deposit event you can see that get_immediate_caller is called to figure out who the depositor is. That function in turn is calling call_stack_element_to_key which returns an Key which is derived either from an AccountHash or ContractPackageHash.

Notice that the underlying representation of these two types is not identical.

The question is whether it makes sense to constrain it to AccountHashes, that would imply that Contracts could not have a kairos/L2 account.

We could probably wrap it in an Either type

I did this on purpose to support both Account Hashs and Contract Hashs, the Key wrapper in my implementation will work well for anything Kairos.

I know we don't like Casper Types but in this case it makes sense to use Key.

@jonas089 If a contract is the depositor how do they make l2 transactions.

The Key is way to broad, it does not work.

To address an l2 account we cannot use casper account hash.
We need the user's public key.
It would be best if we got the public key from the l1 in the Deposit event.

If we cannot get the public key on the l1 in Deposit We could require the user to send it to the deposit endpoint with the deploy we forward.
But that would require going through the l2 to make a deposit which we might not want.

Making the l2 accounts Casper account hashes would require a lot of work and extra in zk compute.

#[repr(C)] #[derive(PartialEq, Eq, Clone, Copy, PartialOrd, Ord, Hash)] #[cfg_attr(feature = "datasize", derive(DataSize))] pub enum Key { /// A `Key` under which a user account is stored. Account(AccountHash), /// A `Key` under which a smart contract is stored and which is the pseudo-hash of the /// contract. Hash(HashAddr), /// A `Key` which is a [`URef`], under which most types of data can be stored. URef(URef), /// A `Key` under which we store a transfer. Transfer(TransferAddr), /// A `Key` under which we store a deploy info. DeployInfo(DeployHash), /// A `Key` under which we store an era info. EraInfo(EraId), /// A `Key` under which we store a purse balance. Balance(URefAddr), /// A `Key` under which we store bid information Bid(AccountHash), /// A `Key` under which we store withdraw information. Withdraw(AccountHash), /// A `Key` variant whose value is derived by hashing [`URef`]s address and arbitrary data. Dictionary(DictionaryAddr), /// A `Key` variant under which system contract hashes are stored. SystemContractRegistry, /// A `Key` under which we store current era info. EraSummary, /// A `Key` under which we store unbond information. Unbond(AccountHash), /// A `Key` variant under which chainspec and other hashes are stored. ChainspecRegistry, /// A `Key` variant under which we store a registry of checksums. ChecksumRegistry, }

There is no way to derive the Public Key from an account hash.

I think your statement that "this doesn't work" is extreme. It definitely works and it might actually be the only solution, it just implies that we will have to hash Public Keys in some places and construct Account Hashs from those Public Keys when parsing the Events.

We can for simplicity restrict it to an account hash, but through call stack we will likely not be able to retrieve a Public Key. I see that this causes computational overhead, but it may not be avoidable unless we find a way to retrieve the Public Key directly from the callstack when the caller is an Account ( and ideally also for another Contract ). I am not aware of such functionality and if none of us find an alternative then I think that we have no choice.

It looks like we will have to use Account Hashs instead of Public Keys for accounting, or at least compute and match the Account Hash when applying Deposits to the L2 state.

But this again is a Casper public key.

Or do we also use Casper public key in the L2?

@koxu1996 I don't think we need to send a full signed tx deposit.
The signing of the tx deposit adds no additional guarantees over what the l1 session provides.
I was thinking we would just use tx for withdraw and transfer, and treat deposit separately since it's l1 initiated and simpler.

We should make this decision asap @koxu1996 @Avi-D-coder so that we don't get blocked here. I'd like to merge this PR into the other contract PR because I think it's getting harder to work with when there are 2 PRs for the same issue.

Also we need the kairos-l1-utils crate which is not included on this branch for some reason. I am also confused over the issue that Andrzej pointed out where it looks like this PR is set to merge into Main.

I agree with Avi I think the easiest would be using the named arg.

@jonas089 @koxu1996 Setting this PR to merge into master serves the purpose of seeing what exact changes would make it in master obviously and to review how the bigger picture would look like.

It moreover triggers the whole pipeline to run since the GHA only run on PRs against master.

@Avi-D-coder Okay, I will stick to non-tx deposit.

@marijanp PR is "deposit contract suggestions", so as the name suggests, it should include suggestions/improvements over contract, not the whole contract.

Avi-D-coder · 2024-05-09T17:19:19Z

kairos-contracts/demo-contract/contract/src/main.rs

+        u64::try_from(amount).unwrap_or_else(|_| runtime::revert(ApiError::InvalidArgument));
+
+    let new_deposit_record: Deposit = Deposit {
+        account: get_immediate_caller().unwrap_or_revert(),


We only use get_immediate_caller once.
Can we inline it and make it more robust?

It is calling call_stack_element_to_key which is a function that is quite deeper. I would say it is fine to keep it factored out.

marijanp · 2024-05-09T17:20:39Z

What's the idea behind this PR? Is it supposed to replace the other deposit contract PR?

The idea is to get the requested changes incorporated and finallize the deposit contract. After we reach a state that addresses all the problems, the idea is to merge this into the feature/deposit-contract branch.

It would be great if you could answer some of the review comments here.

kairos-contracts/demo-contract/contract/src/entry_points.rs

kairos-contracts/demo-contract/contract/src/utils/events.rs

jonas089

lgtm

kairos-contracts/demo-contract/contract/src/utils.rs

koxu1996 · 2024-05-10T08:35:41Z

What's the idea behind this PR? Is it supposed to replace the other deposit contract PR?

The idea is to get the requested changes incorporated and finallize the deposit contract. After we reach a state that addresses all the problems, the idea is to merge this into the feature/deposit-contract branch.

It would be great if you could answer some of the review comments here.

Sounds good, however I am confused by current target branch:

@marijanp Could you update base into feature/deposit-contract?

Right now reviewing PR is more difficult (it includes many more commits), also there is a risk that someone will press "merge" button and merge it directly into main.

marijanp · 2024-05-11T14:30:55Z

@koxu1996 You are able to change the base yourself. An accidental merge was not possible now until I actually changed the base because there was another approver missing. A merge of this would also include all the changes from @jonas089 branch.

koxu1996 · 2024-05-13T09:28:20Z

@koxu1996 You are able to change the base yourself. An accidental merge was not possible now until I actually changed the base because there was another approver missing. A merge of this would also include all the changes from @jonas089 branch.

@marijanp Well... I think we should take responsibility for our own work - I am glad you changed the base. The approver was missing, because I didn't want to let you merge it directly to main 😉. Even though it includes @jonas089's work, I think Jonas’s PR should be approved and merged - not closed and superseded by this "refactoring" PR.

koxu1996

Looks good!

Avi-D-coder

Let's get this merged

jonas089 · 2024-05-13T18:47:36Z

you can merge this, lgtm

edit: I'm done for today and will be back tomorrow morning.

jonas089 · 2024-05-13T21:06:26Z

I resolved the conflicts and the PR is now ready to be merged.

jonas089 · 2024-05-15T14:20:23Z

Is this PR ready to be merged?

marijanp · 2024-05-15T14:57:16Z

@jonas089 not yet, I'm investigating why the GHA fails

marijanp added 10 commits May 3, 2024 14:12

kairos-cli: implement client

bd4792b

kairos-cli: implement call to deposit endpoint and integration test

fa7568a

nixos/end-to-end: use kairos-server address when depositing

2a4916e

kairos-cli: use blocking client

31b4c71

kairos-cli: map to KairosServerError when server returns no-success

9d0ae91

Merge branch 'main' into implement-deposit-cli-3

e63194d

kairos-cli: use clap default_value

0c980ef

Merge branch 'main' into implement-deposit-cli-3

2a5a04a

kairos-server: output the bind address on failure

1da5208

kairos-l1-utils: remove

606fd1d

marijanp added 5 commits May 9, 2024 00:31

demo-contract-tests: refactor fixture and tests

2cd0cb4

Merge pull request #91 from cspr-rad/implement-deposit-cli-3

9ac33ec

kairos-cli: implement call to deposit endpoint and integration test

demo-contract-tests: refactor fixture

f54df4f

demo-contract-tests: add tests to check that admin can't take funds

33ea2fc

demo-contract-tests: remove unused dependency

5f04db2

Avi-D-coder reviewed May 9, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Avi-D-coder reviewed May 9, 2024

View reviewed changes

kairos-contracts/demo-contract/contract/src/main.rs Outdated Show resolved Hide resolved

don't ignore .env

164a60b

Avi-D-coder reviewed May 9, 2024

View reviewed changes

marijanp added 3 commits May 9, 2024 12:24

demo-contract: remove u64::from

34a0738

Merge branch 'main' into deposit-contract-marijan

5a2a899

fix lockfile

496d377

marijanp commented May 9, 2024

View reviewed changes

kairos-contracts/demo-contract/contract/src/entry_points.rs Outdated Show resolved Hide resolved

marijanp added 2 commits May 9, 2024 13:34

demo-contract: account -> depositor

5da25a9

demo-contract: minor init refactor

09da0ad

marijanp commented May 9, 2024

View reviewed changes

kairos-contracts/demo-contract/contract/src/utils/events.rs Outdated Show resolved Hide resolved

marijanp marked this pull request as ready for review May 9, 2024 18:47

demo-contract: remove timestamp from deposit event

b663b89

jonas089 approved these changes May 9, 2024

View reviewed changes

Avi-D-coder reviewed May 9, 2024

View reviewed changes

kairos-contracts/demo-contract/contract/src/utils.rs Outdated Show resolved Hide resolved

marijanp changed the base branch from main to feature/deposit-contract May 11, 2024 14:26

koxu1996 approved these changes May 13, 2024

View reviewed changes

Avi-D-coder approved these changes May 13, 2024

View reviewed changes

marijanp force-pushed the feature/deposit-contract branch from 947e278 to 40822ed Compare May 14, 2024 21:52

marijanp force-pushed the deposit-contract-marijan branch from b163dff to b663b89 Compare May 14, 2024 21:55

marijanp merged commit da68b21 into feature/deposit-contract May 15, 2024
10 checks passed

marijanp deleted the deposit-contract-marijan branch May 15, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deposit contract suggestions #95

Deposit contract suggestions #95

marijanp commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

jonas089 commented May 9, 2024

Avi-D-coder May 9, 2024

marijanp May 9, 2024

jonas089 May 9, 2024 •

edited

Loading

Avi-D-coder May 9, 2024

jonas089 May 9, 2024 •

edited

Loading

jonas089 May 10, 2024 •

edited

Loading

Avi-D-coder May 10, 2024

jonas089 May 10, 2024 •

edited

Loading

marijanp May 11, 2024

koxu1996 May 13, 2024

Avi-D-coder May 9, 2024

marijanp May 9, 2024

marijanp commented May 9, 2024

jonas089 left a comment

koxu1996 commented May 10, 2024 •

edited

Loading

marijanp commented May 11, 2024

koxu1996 commented May 13, 2024

koxu1996 left a comment

Avi-D-coder left a comment

jonas089 commented May 13, 2024 •

edited

Loading

jonas089 commented May 13, 2024

jonas089 commented May 15, 2024

marijanp commented May 15, 2024

Deposit contract suggestions #95

Deposit contract suggestions #95

Conversation

marijanp commented May 8, 2024 • edited Loading

github-actions bot commented May 8, 2024 • edited Loading

jonas089 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas089 May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas089 May 9, 2024 • edited Loading

Choose a reason for hiding this comment

jonas089 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas089 May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marijanp commented May 9, 2024

jonas089 left a comment

Choose a reason for hiding this comment

koxu1996 commented May 10, 2024 • edited Loading

marijanp commented May 11, 2024

koxu1996 commented May 13, 2024

koxu1996 left a comment

Choose a reason for hiding this comment

Avi-D-coder left a comment

Choose a reason for hiding this comment

jonas089 commented May 13, 2024 • edited Loading

jonas089 commented May 13, 2024

jonas089 commented May 15, 2024

marijanp commented May 15, 2024

marijanp commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

jonas089 May 9, 2024 •

edited

Loading

jonas089 May 9, 2024 •

edited

Loading

jonas089 May 10, 2024 •

edited

Loading

jonas089 May 10, 2024 •

edited

Loading

koxu1996 commented May 10, 2024 •

edited

Loading

jonas089 commented May 13, 2024 •

edited

Loading