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

docs: contact classes #36

Merged
merged 1 commit into from
Jun 6, 2022
Merged

docs: contact classes #36

merged 1 commit into from
Jun 6, 2022

Conversation

ArielElp
Copy link
Collaborator

@ArielElp ArielElp commented May 29, 2022

Description of the Changes

Please add a detailed description of the change, whether it's an enhancement or a bugfix.
If the PR is related to an open issue please link to it.

Check List

  • Changes have been done against dev branch, and PR does not conflict
  • Docs have been updated
  • PR title is follow the convention: <docs/feat/fix/chore>(optional scope): <description>, e.g: fix: minor typos in code

This change is Reviewable

@ArielElp ArielElp requested review from bbrandtom and stoobie May 29, 2022 13:22
Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

I still have to review starknet-state.md.

docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Blocks/transactions.md Outdated Show resolved Hide resolved
docs/Contracts/contract-classes.md Outdated Show resolved Hide resolved
docs/Contracts/contract-classes.md Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 11 unresolved discussions (waiting on @bbrandtom and @stoobie)


docs/Blocks/transactions.md line 18 at r1 (raw file):

Previously, stoobie wrote…
| `contract_definition`   | `ContractClass`      | The object that defines the contract's functionality                                |

Done.


docs/Blocks/transactions.md line 25 at r1 (raw file):

Previously, stoobie wrote…
### Calculating the hash of a deploy transaction

Done.


docs/Blocks/transactions.md line 71 at r1 (raw file):

Previously, stoobie wrote…
### Calculating the invoke transaction hash

I used the wording you had for deploy (liked it better + consistency)


docs/Blocks/transactions.md line 89 at r1 (raw file):

Previously, stoobie wrote…
The declare transaction is used to introduce new classes into the state of StarkNet, enabling other contracts to deploy instances of those classes or using them in a library call.

Done.


docs/Blocks/transactions.md line 106 at r1 (raw file):

Previously, stoobie wrote…
### Calculating the declare transaction hash

I used the wording you had for deploy (liked it better + consistency)


docs/Blocks/transactions.md line 118 at r1 (raw file):

Previously, stoobie wrote…
- The placeholders' zeros are used to align the hash computation for the different types of transactions. In this example, they stand for the empty call data and entry point selector.
  1. Why do i need the ' in placeholders?
  2. This is not an example, the "here" specified the locations where we had zeros instead of something meaningful which is there for other types of transactions.

docs/Blocks/transactions.md line 119 at r1 (raw file):

Previously, stoobie wrote…
- `chain_id` is a constant value that specifies the network to which this transaction is sent. See [Chain-Id](./transactions#chain-id).

Done.


docs/Blocks/transactions.md line 124 at r1 (raw file):

Previously, stoobie wrote…
While StarkNet does not have a specific signature scheme built into the protocol, the Cairo language, in which smart contracts are written, does have an efficient implementation for ECDSA signature with respect to a [STARK-friendly curve](../Hashing/hash-functions#stark-curve).

Done.


docs/Blocks/transactions.md line 131 at r1 (raw file):

Previously, stoobie wrote…
[^1]: Be aware that this type of transaction might be deprecated as StarkNet matures, effectively incorporating this into an invoke function transaction over an account contract which will implement the deployment as part of its functionality.

Done.


docs/Contracts/contract-classes.md line 3 at r1 (raw file):

Previously, stoobie wrote…
Taking inspiration from object-oriented programming, StarkNet distinguishes between a contract and its implementation by separating contracts into classes and instances. 

A contract class is the definition of the contract: Cairo byte code, hint information, entry point names, and everything that defines its semantics unambiguously. Each class is identified by its class hash, which is analogous to a class name in an object oriented programming language. A contract instance is a deployed contract corresponding to some class. Notice that only contract instances behave as contracts in that they have their own storage and can be called by transactions or other contracts.

A contract class does not necessarily have a deployed instance in StarkNet.

Done.


docs/Contracts/contract-classes.md line 7 at r1 (raw file):

Previously, stoobie wrote…
New classes can be added to the state of StarkNet with the [`declare`](../Blocks/transactions#declare-transaction) transaction. New instances of a previously declared class can be deployed via the `deploy` system call. 

To use the functionality of a declared class, without deploying an instance of that class, you can use the `library_call` system call. This system call is an analogue of Ethereum's delegate call in the world of classes. You can use class code directly, instead of having a placeholder contract deployed, which is used only for its code.

Done.

Copy link
Member

@bbrandtom bbrandtom left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @ArielElp and @stoobie)


docs/Blocks/transactions.md line 7 at r2 (raw file):

StarkNet, in its Alpha version, supports two types of transactions: a `Deploy` transaction and an `Invoke Function` transaction. We describe the available fields for both of these transaction types and how the transaction hash is calculated in both cases.

## Deploy transaction

please add a callout that this tx will be deprecated in the future and users should stop using it.


docs/Blocks/transactions.md line 21 at r2 (raw file):

| `constructor_calldata`  | `List<FieldElement>` | The arguments passed to the constructor during deployment                        |
| `caller_address`        | `FieldElement`       | Who invoked the deployment. Set to 0 (in future: the deploying account contract) |
| `version`               | `FieldElement`       | The intended StarkNet OS version                                                 |

I don't think this is the purpose of this field.
The version is the version of the tx, it allows us to create new txs type. i.e., there might be an OS version that supports two txs versions.
Same for all descriptions of this field below.

Code quote:

 The intended StarkNet OS version 

docs/Blocks/transactions.md line 37 at r2 (raw file):

$$

Where:

explain what is the placeholder 0


docs/Blocks/transactions.md line 89 at r2 (raw file):

## Declare transaction

The declare transaction is used to introduce new classes into the state of StarkNet, enabling other contracts to deploy instances of those classes or using them in a library call.

please ref to the contract classes page.


docs/Blocks/transactions.md line 97 at r2 (raw file):

| Name             | Type                 | Description                                                                               |
| ---------------- | -------------------- | ----------------------------------------------------------------------------------------- |
| `contract_class` | `ContractClass`      | The class object                                                                          |

why do we call it contract_class here and contract definition for the deploy tx? It is the same object right?


docs/Blocks/transactions.md line 112 at r2 (raw file):

$$
\begin{aligned}
\text{invoke\_tx\_hash} = h( & \text{"declare"}, \text{version}, \text{sender\_address}, \\& 0, 0, \text{max\_fee}, \text{chain\_id}, \text{class\_hash})

isn't the nonce missing here?

Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @bbrandtom and @stoobie)


docs/Blocks/transactions.md line 7 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

please add a callout that this tx will be deprecated in the future and users should stop using it.

Done.


docs/Blocks/transactions.md line 21 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

I don't think this is the purpose of this field.
The version is the version of the tx, it allows us to create new txs type. i.e., there might be an OS version that supports two txs versions.
Same for all descriptions of this field below.

Done.


docs/Blocks/transactions.md line 37 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

explain what is the placeholder 0

Done.


docs/Blocks/transactions.md line 89 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

please ref to the contract classes page.

Done.


docs/Blocks/transactions.md line 97 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

why do we call it contract_class here and contract definition for the deploy tx? It is the same object right?

it is, but it's still named "definition" in the transaction structure, so I think we should keep it until deploy is deprecated


docs/Blocks/transactions.md line 112 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

isn't the nonce missing here?

Not included yet

bbrandtom
bbrandtom previously approved these changes Jun 6, 2022
Copy link
Member

@bbrandtom bbrandtom left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @stoobie)


docs/Blocks/transactions.md line 112 at r2 (raw file):

Previously, ArielElp wrote…

Not included yet

seems like a bug no? in any case, unblocking

Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 11 unresolved discussions (waiting on @stoobie)


docs/Blocks/transactions.md line 112 at r2 (raw file):

Previously, bbrandtom (Tom Brand) wrote…

seems like a bug no? in any case, unblocking

Since the same logic is used for all transaction types, I think they delayed it to the nonce related changes.

Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

LGTM. One minor comment.

@@ -26,6 +30,10 @@ Where:
- $\text{storage\_root}$ is the root of another Merkle-Patricia tree of height 251 that is constructed from the contract’s storage
- $h$ is the [Pedersen](../Hashing/hash-functions#pedersen-hash) hash function.

:::info
Note that, as of the time of writing, the commitment does not include classes information. This will be added in future versions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Note that, as of the time of writing, the commitment does not include classes information. This will be added in future versions.
Currently, the commitment does not include information for classes. This information will be added in future versions.

Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 11 unresolved discussions (waiting on @ArielElp and @stoobie)


docs/Blocks/transactions.md line 118 at r1 (raw file):

Previously, ArielElp wrote…
  1. Why do i need the ' in placeholders?
  2. This is not an example, the "here" specified the locations where we had zeros instead of something meaningful which is there for other types of transactions.
  1. I understand that you're referring to the zeros belonging to the placeholders. Do I misunderstand? If I'm right, than you need an apostrophe to indicate ownership. The apostrophe is after the "s" because I understood that "placeholders" is plural. If you mean a single placeholder, then it should be "placeholder's zeros".

docs/Blocks/transactions.md line 10 at r4 (raw file):

:::caution
The deploy transaction will be deprecated in future StarkNet versions. To deploy new constract instances, you can use the `deploy` syscall. For more information, see [contract classes](../Contracts/contract-classes).

My understanding of "deprecated" is: It will be removed/unsupported in the future, so you should already start to stop using this.

Are you saying it will be deprecated in the future, or removed in the future?

Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 11 unresolved discussions (waiting on @ArielElp and @stoobie)


docs/Blocks/transactions.md line 118 at r1 (raw file):

Previously, stoobie (Steve Goodman) wrote…
  1. I understand that you're referring to the zeros belonging to the placeholders. Do I misunderstand? If I'm right, than you need an apostrophe to indicate ownership. The apostrophe is after the "s" because I understood that "placeholders" is plural. If you mean a single placeholder, then it should be "placeholder's zeros".

The issue is the following: if you look at the transaction hash calculation, there are a bunch of zeros, and it's unclear what purpose they serve.

The motivation was to have the same hash structure across all transaction types (you can see that all hashes have 7 arguments total). However, some fields don't make sense for all the transactions, e.g. calldata for a declare transaction, so we just have zero (in that sense it's just a placeholder).

Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 11 unresolved discussions (waiting on @stoobie)


docs/Blocks/transactions.md line 10 at r4 (raw file):

Previously, stoobie (Steve Goodman) wrote…

My understanding of "deprecated" is: It will be removed/unsupported in the future, so you should already start to stop using this.

Are you saying it will be deprecated in the future, or removed in the future?

The former, but for some usecases (e.g. account deployment), it's the only possible option today. So, when possible, we want you to use the deploy syscall.

@ArielElp ArielElp merged commit 30a3204 into dev Jun 6, 2022
@ArielElp ArielElp deleted the ariel/contract_classes branch June 6, 2022 09:55
Copy link
Collaborator Author

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 12 unresolved discussions


docs/State/starknet-state.md line 34 at r4 (raw file):

Previously, stoobie (Steve Goodman) wrote…
Currently, the commitment does not include information for classes. This information will be added in future versions.

Re "information for classes"

The commitment is not a tool to obtain information from, but just a digest of the data (essentially a Merkle root). When I say it has no classes information, I mean that we currently do not commit to the classes part of the state (classes information is not incorporated in the commitment)

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.

3 participants