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

add v3 transactions and missing fields to receipts & block header #131

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

ArielElp
Copy link
Collaborator

@ArielElp ArielElp commented Sep 2, 2023

This change is Reviewable

"description": "Version of the transaction scheme",
"type": "string",
"enum": [
"0x2"

Choose a reason for hiding this comment

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

Shouldn't it be 0x3? 👀

"execution_status": {
"title": "Execution status",
"$ref": "#/components/schemas/TXN_EXECUTION_STATUS"
"execution_reources": {

Choose a reason for hiding this comment

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

👀

Choose a reason for hiding this comment

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

Typo, I think it should be "execution_resources"

Copy link
Collaborator

@EvyatarO EvyatarO left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 21 unresolved discussions (waiting on @ArielElp and @DelevoXDG)


api/starknet_api_openrpc.json line 1370 at r1 (raw file):

                    },
                    "l1_gas_price": {
                        "title": "L1 gas price",

You need to have to gas price fields (eth and strk)


api/starknet_api_openrpc.json line 1799 at r1 (raw file):

            },
            "DECLARE_TXN_V3": {
                "title": "Declare Transaction V2",

V3?


api/starknet_api_openrpc.json line 1800 at r1 (raw file):

            "DECLARE_TXN_V3": {
                "title": "Declare Transaction V2",
                "description": "Declare Contract Transaction V2",

V3?


api/starknet_api_openrpc.json line 1814 at r1 (raw file):

                            },
                            "sender_address": {
                                "title": "Sender address",

Don't you want to use the common fields in one place?


api/starknet_api_openrpc.json line 1823 at r1 (raw file):

                                "$ref": "#/components/schemas/FELT"
                            },
                            "max_fee": {

no max_fee


api/starknet_api_openrpc.json line 1833 at r1 (raw file):

                                "type": "string",
                                "enum": [
                                    "0x2"

0x3


api/starknet_api_openrpc.json line 1854 at r1 (raw file):

                                "$ref": "#/components/schemas/RESOURCE_LIMITS"
                            },
                            "l2_gas": {

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
Same for tip, da_mode and init_data


api/starknet_api_openrpc.json line 1880 at r1 (raw file):

                            }
                        },
                        "required": [

There is no max_fee
l2_gas, tip, paymaster_address,nonce and fee data_availability mode are not requires ATM


api/starknet_api_openrpc.json line 1920 at r1 (raw file):

                    },
                    {
                        "title": "Broadcasted invoke transaction V2",

Why v2?


api/starknet_api_openrpc.json line 2058 at r1 (raw file):

                    {
                        "type": "object",
                        "title": "Declare txn v2",

V3?


api/starknet_api_openrpc.json line 2078 at r1 (raw file):

                            },
                            "max_fee": {
                                "title": "Max fee",

Not exist in v3


api/starknet_api_openrpc.json line 2106 at r1 (raw file):

                            },
                            "l2_gas": {
                                "title": "L2 Gas",

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
(Same for tip, volition fields, init code)


api/starknet_api_openrpc.json line 2131 at r1 (raw file):

                            }
                        },
                        "required": [

See comment in invoke


api/starknet_api_openrpc.json line 2235 at r1 (raw file):

                    },
                    "max_fee": {
                        "title": "Max fee",

Doesn't exist in v3


api/starknet_api_openrpc.json line 2276 at r1 (raw file):

                    },
                    "l2_gas": {
                        "title": "L2 Gas",

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
(Same for tip, volition fields, init code)


api/starknet_api_openrpc.json line 2301 at r1 (raw file):

                    }
                },
                "required": [

See comment above


api/starknet_api_openrpc.json line 2493 at r1 (raw file):

                                ]
                            },
                            "sender_address": {

Don't you want to create an object for common fields?


api/starknet_api_openrpc.json line 2523 at r1 (raw file):

                                "$ref": "#/components/schemas/RESOURCE_LIMITS"
                            },
                            "l2_gas": {

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
(Same for tip, volition fields, init code)


api/starknet_api_openrpc.json line 3579 at r1 (raw file):

                "properties": {
                    "max_amount": {
                        "title": "max amount",

max_amount_per_unit?

@tomek0123456789
Copy link

Just reminding that starknet_traceBlockTransactions should accept BLOCK_ID instead of BLOCK_HASH

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.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @DelevoXDG and @EvyatarO)


api/starknet_api_openrpc.json line 1370 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

You need to have to gas price fields (eth and strk)

Added (still need to add the exact unit in the description; are we going with 10^-18?)


api/starknet_api_openrpc.json line 1799 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

V3?

Done.


api/starknet_api_openrpc.json line 1800 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

V3?

Done.


api/starknet_api_openrpc.json line 1823 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

no max_fee

Done.


api/starknet_api_openrpc.json line 1833 at r1 (raw file):

Previously, DelevoXDG (Maksim Zdobnikau) wrote…

Shouldn't it be 0x3? 👀

Done.


api/starknet_api_openrpc.json line 1833 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

0x3

Done.


api/starknet_api_openrpc.json line 1854 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
Same for tip, da_mode and init_data

Removed all. We should add them as required in the corresponding version.


api/starknet_api_openrpc.json line 1880 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

There is no max_fee
l2_gas, tip, paymaster_address,nonce and fee data_availability mode are not requires ATM

Done.


api/starknet_api_openrpc.json line 1920 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

Why v2?

Done.


api/starknet_api_openrpc.json line 2058 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

V3?

Done.


api/starknet_api_openrpc.json line 2078 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

Not exist in v3

Done.


api/starknet_api_openrpc.json line 2106 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
(Same for tip, volition fields, init code)

Deleted until it's actually relevant


api/starknet_api_openrpc.json line 2131 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

See comment in invoke

Done.


api/starknet_api_openrpc.json line 2235 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

Doesn't exist in v3

Done.


api/starknet_api_openrpc.json line 2276 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
(Same for tip, volition fields, init code)

Done.


api/starknet_api_openrpc.json line 2301 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

See comment above

Done.


api/starknet_api_openrpc.json line 2523 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

it's part of v3 but you shouldn't send it on v0.13.0, should we keep it here in the meantime?
(Same for tip, volition fields, init code)

Done.


api/starknet_api_openrpc.json line 3579 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

max_amount_per_unit?

What's max_amount_per_unit? ATM there's max_amount and max_price_per_unit

Copy link
Collaborator

@EvyatarO EvyatarO left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArielElp and @DelevoXDG)


api/starknet_api_openrpc.json line 1370 at r1 (raw file):

Previously, ArielElp wrote…

Added (still need to add the exact unit in the description; are we going with 10^-18?)

We are going w/ 10^-18, still no name yet.


api/starknet_api_openrpc.json line 3579 at r1 (raw file):

Previously, ArielElp wrote…

What's max_amount_per_unit? ATM there's max_amount and max_price_per_unit

You are correct.

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.

Dismissed @EvyatarO from 4 discussions.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @DelevoXDG and @EvyatarO)


api/starknet_api_openrpc.json line 1814 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

Don't you want to use the common fields in one place?

These objects used to be less readable when non-flat, I rather keep it flat for now until we can completely remove old transaction types.


api/starknet_api_openrpc.json line 2493 at r1 (raw file):

Previously, EvyatarO (Evyatar) wrote…

Don't you want to create an object for common fields?

These objects used to be less readable when non-flat, I rather keep it flat for now until we can completely remove old transaction types.


api/starknet_api_openrpc.json line 3043 at r1 (raw file):

Previously, DelevoXDG (Maksim Zdobnikau) wrote…

👀

?

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.

:lgtm:

Reviewed 2 of 3 files at r3, 1 of 1 files at r4.
Dismissed @DelevoXDG from a discussion.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @ArielElp)


api/starknet_api_openrpc.json line 3043 at r1 (raw file):

Previously, DelevoXDG (Maksim Zdobnikau) wrote…

Typo, I think it should be "execution_resources"

Ah true, thanks!

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.

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

@ArielElp ArielElp merged commit cddef47 into master Sep 13, 2023
1 check passed
@ArielElp ArielElp deleted the v0.5.0 branch September 13, 2023 16:56
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.

4 participants