add deposit contract crates to workspace #29

jonas089 · 2024-02-29T17:00:56Z

There is a dedicated crate for contract tests
The deposit smart contract fetches node and casper-client dependencies from cspr-rad/casper-node & cspr-rad/casper-client

The compile-contracts.sh script can be used to build all session code that is used for testing, as well as the smart contract itself.

The PATH_TO_WASM_BINARIES environment variable must be set before running the tests. An example can be found in the deposit contract's readme.md

The deployment target is the latest feature branch of the Casper node 2.0 (28th of Feb 2024) and a seemingly stable release of casper-client. Contract deployment was tested/ can be tested using the kairos-cctl-env.

koxu1996 · 2024-03-04T14:47:55Z

The deposit smart contract fetches node and casper-client dependencies from cspr-rad/casper-node & cspr-rad/casper-client

@jonas089 What is exactly changed in the custom version of casper-node and casper-client? Btw. client does not seem to be used.

koxu1996 · 2024-03-04T15:15:10Z

kairos-deposit-contract/compile-contracts.sh

+rustup target add wasm32-unknown-unknown
+
+cargo build -p contract --release --target wasm32-unknown-unknown
+wasm-opt --strip-debug --signext-lowering ./target/wasm32-unknown-unknown/release/deposit-contract.wasm -o ./target/wasm32-unknown-unknown/release/deposit-contract-optimized.wasm


This introduces wasm-opt dependency that has to be manually installed 😒.

I believe we can remove it from this bash script, and include wasm_opt crate in the tests, that will be used for creating optimized file side-by-side (unfortunately the library does not allow working directly on memory).

Installing wasm-opt system wide is a viable option imo. Anyone working with casper contracts should have it available anyways. Makefiles in other contracts rely on it as well and it is as long as it works I don't see a need to change anything about it.

Thanks to Nix I'm not too worried about this

Anyone working with casper contracts should have it available anyways.

@jonas089 Wrong. You have it installed, because I showed you this can be used for fixing opcode 192 issue and using most recent Rust complier. By default Casper contracts rely on wasm-strip (binary from wabt), so I am still insisting on using crate instead of unknown system binary.

Since you know exactly what to do here and I don't, would it be possible for you to submit a PR over this issue? Otherwise I can look into it later today.

kairos-deposit-contract/Cargo.toml

jonas089 · 2024-03-04T15:33:31Z

The deposit smart contract fetches node and casper-client dependencies from cspr-rad/casper-node & cspr-rad/casper-client

@jonas089 What is exactly changed in the custom version of casper-node and casper-client? Btw. client does not seem to be used.

Casper-client is used in my contract-cli / library that I excluded from this PR as per your request.
The casper-node and casper-client are forked so that we have a version-locked development branch for Kairos.
I did some quality of life improvements that you can take a look at, but these details are not important for Kairos development.

koxu1996 · 2024-03-04T15:36:44Z

The deposit smart contract fetches node and casper-client dependencies from cspr-rad/casper-node & cspr-rad/casper-client

@jonas089 What is exactly changed in the custom version of casper-node and casper-client? Btw. client does not seem to be used.

Casper-client is used in my contract-cli / library that I excluded from this PR as per your request. The casper-node and casper-client are forked so that we have a version-locked development branch for Kairos. I did some quality of life improvements that you can take a look at, but these details are not important for Kairos development.

I would like to know what exactly are the improvements, because if the only change is single WASM path, then I think it's not worth maintaining the custom version of node (+ client).

jonas089 · 2024-03-04T15:39:17Z

Member

I disagree. With the current workspace configuration it is best to version lock and keep my change to the wasm path. If you want to review my changes to the kairos-* branches feel free to do so, but I currently have no intention to change them since the contract environment is workable and will change frequently.

koxu1996 · 2024-03-04T15:52:48Z

Pardon me, but I see that using custom node is useless now (if the only change is WASM path). If there is no reason to maintain custom fork, then I am heavily against relying on it. It would be helpful if you could describe those changes, so everyone could understand why we need them.

I understand your frustration with the amount of work to make changes nice and clean, but one of the project pillars was to focus on clean and production ready code.

jonas089 · 2024-03-04T16:05:26Z

Pardon me, but I see that using custom node is useless now (if the only change is WASM path). If there is no reason to maintain custom fork, then I am heavily against relying on it. It would be helpful if you could describe those changes, so everyone could understand why we need them.

I understand your frustration with the amount of work to make changes nice and clean, but one of the project pillars was to focus on clean and production ready code.

We clearly don't have the same definition of clean code on this matter. We are in a development environment with no definite build target, therefore I don't see a need to be this precise when it comes to the build target for prototyping. You didn't want me to use commit hashs in the Cargo.toml and suggested to version-lock by creating a branch under cspr-rad. This is what I did and yes it cost me a significant amount of time to achieve the same result. I changed the wasm output path in the test engine because on the main repo it is derived from a workspace constant that is set by cargo automatically. This is only useful when the contract is a seperate project and not when the tests are included in a higher-level workspace as is the case with kairos. The change is non-breaking and the test engine has no impact on the production wasm code execution. We are definitely better off with a version locked target since breaking changes can occur on the feature branch and pointing to the main repo will be a huge pain. We want clean code, but we also want a working prototype. We need to make some compromises in the beginning as we can spend a lot of time refining when the fundamentals are in place.

koxu1996 · 2024-03-04T17:10:47Z

@jonas089 I didn't request any changes yet. Before I can submit review, I would like to understand your changes - especially what needs to be adjusted in casper-node to make it work for Kairos, because I believe we can simply avoid it. Please help me understanding it, then we will get it merged sooner.

Regarding commit-locking, I think you should see Specifying dependencies from git repositories. Changes happening on the branch will not break our code, as they are locked with Cargo.lock 😉:

Once a git dependency has been added, Cargo will lock that dependency to the latest commit at the time. New commits will not be pulled down automatically once the lock is in place. However, they can be pulled down manually with cargo update.

jonas089 · 2024-03-04T17:31:24Z

Someone could override Cargo.lock or run cargo update, I think a seprate branch is more secure.

jonas089 · 2024-03-04T17:40:37Z

@jonas089 I didn't request any changes yet. Before I can submit review, I would like to understand your changes - especially what needs to be adjusted in casper-node to make it work for Kairos, because I believe we can simply avoid it. Please help me understanding it, then we will get it merged sooner.

Regarding commit-locking, I think you should see Specifying dependencies from git repositories. Changes happening on the branch will not break our code, as they are locked with Cargo.lock 😉:

Once a git dependency has been added, Cargo will lock that dependency to the latest commit at the time. New commits will not be pulled down automatically once the lock is in place. However, they can be pulled down manually with cargo update.

As far as the Casper-Node goes the only change that I had to make is the one in the execution-engine-tests regarding the wasm path (and the fact that I would rather lock the version by branch than rely on Cargo.lock).

koxu1996

As far as the Casper-Node goes the only change that I had to make is the one in the execution-engine-tests regarding the wasm path (and the fact that I would rather lock the version by branch than rely on Cargo.lock).

Okay, I am glad we confirmed it's the only change. We can untangle this then 😃, please follow. I guess you customized casper-node in order to make this code works in the test_fixture.rs:

    pub fn install(&mut self, admin: AccountHash) {
        ...
        let install_contract_request =
            ExecuteRequestBuilder::standard(admin, "deposit-contract-optimized.wasm", session_args)
                .build()

If you check how WASM paths are resolved underneath, you will see that it:

Firstly, if path is relative, it goes through the predefined paths (including the one you customized) and check if file exists.
Secondly, it checks if file exists at given relative path (to the current directly) or absolute path.

💡 So there is no point in customizing predefined paths, just provide valid relative/absolute path directly in tests.

koxu1996 · 2024-03-05T14:48:24Z

Someone could override Cargo.lock or run cargo update, I think a seprate branch is more secure.

Forking repository just for locking changes does not make sense. Just use Cargo's lock branch; or even rev if you are really worried that someone who is updating dependencies does not expect to update dependencies.

marijanp · 2024-03-06T06:19:23Z

kairos-deposit-contract/compile-contracts.sh

@@ -0,0 +1,16 @@
+rustup target add wasm32-unknown-unknown


The shebang is missing. What shell is this script supposed to be compatible with?

bash, when could the missing shebang cause issues?

When someone uses another default shell than bash? Someone could be running fish per default for example.

kairos-deposit-contract-tests/Cargo.toml

marijanp · 2024-03-06T06:32:11Z

kairos-deposit-contract-tests/src/integration_tests.rs

+
+    fn setup() -> (TestContext, AccountHash) {
+        let fixture: TestContext = TestContext::new();
+        let installer = fixture.account_1;


What is the reason for aliasing account_1 with installer? The tests read that the installer is depositing X amount of tokens

marijanp · 2024-03-06T06:35:48Z

kairos-deposit-contract-tests/src/test_fixture/utils.rs

+    PublicKey, SecretKey,
+};
+
+// Creates a dummy account and transfer funds to it


Explain what happens if the account_string is None. This is information that helps a user understand what happens in special cases. If not documented I have to read the code instead. but [7u8; 32] is not very informative.

Since this is the tests crate, the case where the account_string is None doesn't need to be handled. We just need a set of n dummy accounts to work with, there are no edgecases.

That is exactly what I mean. I'm pointing out that these non-obvious conditions need to be documented, such that any developer using or extending this function understands the intention. I didn't know this, and I still dont understand what the None case does in case the account string is None.
If you say that the None case is impossible, then it would be better to design the interface that way or maybe use expect

utils.rs is not really supposed to be touched by smart contract developers who are writing tests. It is used to setup test accounts and stuff and we won't need to touch it for Kairos related work. I didn't write utils.rs myself and don't see an immediate need to improve it. I suggest that we keep your suggestions in mind and revisit this in case we decide to touch utils.rs.

marijanp · 2024-03-06T06:36:57Z

kairos-deposit-contract/compile-contracts.sh

+rustup target add wasm32-unknown-unknown
+
+cargo build -p contract --release --target wasm32-unknown-unknown
+wasm-opt --strip-debug --signext-lowering ./target/wasm32-unknown-unknown/release/deposit-contract.wasm -o ./target/wasm32-unknown-unknown/release/deposit-contract-optimized.wasm


Thanks to Nix I'm not too worried about this

marijanp · 2024-03-06T07:00:17Z

kairos-deposit-contract/malicious-session/src/main.rs

+#![no_main]
+use casper_contract::contract_api::{account, runtime, system};
+use casper_types::{runtime_args, AddressableEntityHash, URef, U512};
+


Can we add a comment here on what a malicious-session does/ is supposed to do?

I will add a comment explaining the session codes on top of each file.

marijanp · 2024-03-06T07:02:47Z

kairos-deposit-contract/malicious-session/Cargo.toml

missing newline at the end of file. I will stop pointing it out, the github diffs point it out

Is there a quick and easy fix or will I have to do it manually? I ran nix fmt but apparently that doesn't help here?

I've added an editorconfig to this repository. Your editor should be able to pick that up and add these newlines.

kairos-deposit-contract/malicious-reader/src/main.rs

marijanp · 2024-03-06T07:05:48Z

kairos-deposit-contract/deposit-session/src/main.rs

+    let temp_purse: URef = system::create_purse();
+    // fund the temporary purse
+    system::transfer_from_purse_to_purse(source, temp_purse, amount, None).unwrap();
+    // call the deposit endpoint


the code literally says call_contract(..."deposit")

The entry point name is static, I can introduce a constant if you prefer that. Something like DEPOSIT_EP_NAME = "deposit".

I haven't been clear enough with my comment: I mean documentation should be used to point out non-obvious things. This comment is repeating the code.

Wouldn't it be easier to suggest a change directly than open a conversation in cases where you only want a single line to be removed or edited?

marijanp · 2024-03-06T07:07:31Z

kairos-deposit-contract/deposit-session/src/main.rs

+    let contract_hash: AddressableEntityHash = runtime::get_named_arg("deposit_contract");
+    let amount: U512 = runtime::get_named_arg("amount");
+    let source: URef = account::get_main_purse();
+    // create a temporary purse that can be passed to the deposit contract


It would be good if you can explain what purpose the temporary purse serves. Do we need an intermediate purse to deposit to our contract's purse? is it then owner_purse -> tem_purse -> contract_purse

This is a Casper-related inconvenience. We can't pass the user's purse directly so we need to create a temporary purse, fund it and pass it to the contract. The contract then empties the funded purse and transfers all funds to its own purse.

Please document this here such that someone revisiting this code is aware

marijanp · 2024-03-11T13:04:05Z

kairos-deposit-contract/readme.md

+
+For now, on Macos:
+
+`./build-macos-darwin-feat-2.0.sh`


This file is missing

add deposit contract crates to workspace

df5c3f9

koxu1996 self-requested a review March 4, 2024 13:41

koxu1996 reviewed Mar 4, 2024

View reviewed changes

kairos-deposit-contract/Cargo.toml Show resolved Hide resolved

koxu1996 requested changes Mar 5, 2024

View reviewed changes

marijanp suggested changes Mar 6, 2024

View reviewed changes

jonas089 added 2 commits March 7, 2024 15:30

Update Cargo.toml

95a8594

Remove unwanted comment in main.rs

8953d54

marijanp assigned jonas089 Mar 11, 2024

marijanp reviewed Mar 11, 2024

View reviewed changes

kairos-deposit-contract/readme.md

For now, on Macos:

`./build-macos-darwin-feat-2.0.sh`

Copy link

Contributor

marijanp Mar 11, 2024

Choose a reason for hiding this comment

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

This file is missing

marijanp added the demo Required for our first demo label Mar 21, 2024

jonas089 closed this Mar 25, 2024

jonas089 deleted the feature/deposit-contract branch March 25, 2024 17:05

koxu1996 mentioned this pull request Mar 27, 2024

Improved version of Deposit contract #51

Merged

add deposit contract crates to workspace #29

add deposit contract crates to workspace #29

Conversation

jonas089 commented Feb 29, 2024

koxu1996 commented Mar 4, 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

jonas089 commented Mar 4, 2024

koxu1996 commented Mar 4, 2024

jonas089 commented Mar 4, 2024

koxu1996 commented Mar 4, 2024

jonas089 commented Mar 4, 2024

koxu1996 commented Mar 4, 2024

jonas089 commented Mar 4, 2024

jonas089 commented Mar 4, 2024

koxu1996 left a comment

Choose a reason for hiding this comment

koxu1996 commented Mar 5, 2024

Choose a reason for hiding this comment

jonas089 Mar 6, 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

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

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

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

Choose a reason for hiding this comment

koxu1996 commented Mar 4, 2024 •

edited

Loading

jonas089 Mar 6, 2024 •

edited

Loading