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

Improved README #106

Merged
merged 4 commits into from
May 16, 2023
Merged

Improved README #106

merged 4 commits into from
May 16, 2023

Conversation

hashedone
Copy link
Collaborator

There is some bug with multitest generation which makes it invalid right now - figuring it out.

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #106 (47815af) into main (966ecb4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #106   +/-   ##
=======================================
  Coverage   66.09%   66.09%           
=======================================
  Files          28       28           
  Lines        1926     1926           
=======================================
  Hits         1273     1273           
  Misses        653      653           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hashedone
Copy link
Collaborator Author

Closes #102 #103

README.md Outdated
chains' custom messages, so it is possible to use it with custom blockchains - except for handling custom
queries and sudo messages. All messages can be generic, so creating a contract eligible to work
across several different CosmWasm blockchains is also possible.
Fist you need your contract crate, which should be a library crate:
Copy link
Collaborator

Choose a reason for hiding this comment

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

First typo


```

To use sylvia in the contract, you need to add couple dependencies - sylvia itself,
Copy link
Collaborator

Choose a reason for hiding this comment

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

a couple of dependencies

README.md Outdated
and additionally: `cosmwasm-schema`, `schemars` and `cosmwasm_std`.

```shell
$ cargo add syvia cosmwasm-schema schemars cosmwasm-std
Copy link
Collaborator

Choose a reason for hiding this comment

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

sylvia

README.md Outdated
To build your contract as wasm you can use:

```rust
$ targo build --target wasm32-unknown-unknown
Copy link
Collaborator

Choose a reason for hiding this comment

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

cargo

README.md Outdated
```

To use sylvia in the contract, you need to add couple dependencies - sylvia itself,
and additionally: `cosmwasm-schema`, `schemars` and `cosmwasm_std`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

also serde is required I think

README.md Outdated
Comment on lines 194 to 197
function argument. Be careful using the typical Rust pattern to prefix it with
`_` to leave it unused. Unfortunately, to properly silence the unused warning here,
you need to add the `#[allow(unused)]` attribute before the argument or the whole
function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding _ prefix should work. It is f.e. done in messages_generation.rs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I completely forgot it finally get to be implemented

README.md Outdated
it is very easy to do:

```rust
```rust
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicated

README.md Outdated

## Interfaces

One of the fundamental ideas of Sylvia's framework is interfaces, allowing the
Copy link
Collaborator

Choose a reason for hiding this comment

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

are

README.md Outdated
.call(owner);

contract
.group()
Copy link
Collaborator

Choose a reason for hiding this comment

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

group_proxy()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really don't like this proxy suffix tbh :/ Just because we named type this way doesn't have to make the call have some suffixed added. Anyway - we will get rid of this call as soon as #109 is done.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice docs and explanation.

Added some minor comments, but generally good to merge when addressed.

The other meta-issue, is this is huge, and maybe best to break into 4-5 different pages and link them from a directory in the README. Keeping the intro explanation, then links to how to use, then the Roadmap. Maybe a few other key points in the README, but keeping the detailed codingg instructions linked from README more like a book.

(I prefer the docs here than in book now, as easier to find, and the code is changing so wil need to be updated with future PRs)

...
```

You should also make sure your crate is compiling as `cdylib`, setting the proper
Copy link
Member

Choose a reason for hiding this comment

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

This is useful instructions. But two points:

  1. I would break "create a new contract" into it's own markdown file (linked from this)
  2. People shouldn't do all these steps, but could do something like cw-template to generate a basic sylvia contract (feel free to make it simpler / targetted to multi-contract repos)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Splitting it into different files is definitely the way to go. However API is not stabilized yet, and it would be easier to check one file before release to update it, than the whole set of them.

Cargo template is a good idea. Just created #110. Will be part of 0.5 (I really don't want to add anything to 0.4).

README.md Outdated

```rust
struct InstantiateMsg {
counter: 64,
Copy link
Member

Choose a reason for hiding this comment

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

typo - u64

README.md Outdated
```

One problem you might face now is that we use the `StdResult` for our contract,
but we often want to define the custom error type for our contracts - hopefully,
Copy link
Member

Choose a reason for hiding this comment

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

As in CosmWasm/book#39... it's fortunately (not hopefully)


Here are a couple of things to talk about.

First, note that I defined the interface trait in its separate module with a name
Copy link
Member

Choose a reason for hiding this comment

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

Please explain this a bit more in detail with examples:

mod group -> trait Group
mod cross_staking -> trait CrossStaking

use sylvia::cw_std::{WasmMsg, to_binary};

fn some_handler(my_contract_addr: String) -> StdResult<Response> {
let msg = my_contract_crate::Execute::Increment {};
Copy link
Member

Choose a reason for hiding this comment

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

This isn't my_contract_crate::MyContractExecMsg::Increment {} ??

I am seeing stuff like this when coding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually typo - it is my_contract_crate::ExecMsg::Increment {}

```rust
use sylvia::cw_std::{WasmMsg, to_binary};

fn some_handler(my_contract_addr: String) -> StdResult<Response> {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this easier with #28 and you can update this code to show how to use the handlers...
Not blocking a merge, but a note for later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#28 is not yet implemented. It Is README for the state of today. Will be updated before 0.5 release

}
```

First of all, note the `contract` module I am using here - it is a slight change
Copy link
Member

Choose a reason for hiding this comment

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

Very nice explanation of multi-test usage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately not true before 0.4 because of #108 has to be first merged.

@hashedone hashedone merged commit d62c205 into main May 16, 2023
6 checks passed
@hashedone hashedone deleted the documentation-improve branch May 16, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants