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

rework precompile tutorial #1231

Merged
merged 34 commits into from
Feb 25, 2023
Merged

rework precompile tutorial #1231

merged 34 commits into from
Feb 25, 2023

Conversation

ceyonur
Copy link
Contributor

@ceyonur ceyonur commented Feb 21, 2023

Reworks precompile generator tutorial for recent upcoming changes in ava-labs/subnet-evm#389

Also adde AllowList capability to tutorial precompile.

@ceyonur ceyonur added DO NOT MERGE This PR is not meant to be merged in its current state subnets labels Feb 21, 2023
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 21, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8d03198
Status: ✅  Deploy successful!
Preview URL: https://261ef9e4.avalanche-docs.pages.dev
Branch Preview URL: https://precompile-improvements.avalanche-docs.pages.dev

View logs

[`./precompile/params.go`](https://github.com/ava-labs/subnet-evm/blob/HelloWorldOfficialTutorial/precompile/params.go)
and modify the default value to be the next user available stateful precompile address. For forks of
We can see the `ContractAddress` is set to some default value.
You should change that default value to a suitable address for your precompile.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be consistent between we and you and what person we are using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed some places

Comment on lines 574 to 576
`Verify()` is called on startup and an error is treated as fatal. Generated code contains a call
to `AllowListConfig.Verify()` to verify the `AllowListConfig`. You can leave that as is, and start
adding your own custom verify code after that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Verify()` is called on startup and an error is treated as fatal. Generated code contains a call
to `AllowListConfig.Verify()` to verify the `AllowListConfig`. You can leave that as is, and start
adding your own custom verify code after that.
`Verify()` is called on startup and an error is treated as fatal. Generated code contains a call
to `AllowListConfig.Verify()` to verify the `AllowListConfig.` You can leave that as is and start
adding your own custom verify code after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to keep . outside of ticks.

By default these are added to the default gas costs to the state-change functions (SetGreeting)
of the precompile. Meaning that these functions will cost an additional `ReadAllowListGasCost` in order
to read permissions from the storage. If you don't plan to read permissions from the storage then
you can omit these.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we talk about the dangers of not accurately putting the correct gas costs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, added

_ "github.com/ava-labs/subnet-evm/precompile/contracts/helloworld"
// ADD YOUR PRECOMPILE HERE
// _ "github.com/ava-labs/subnet-evm/precompile/contracts/yourprecompile"
)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Comment on lines 930 to 935
Let's add some unit tests to make sure our precompile is configured correctly. Config tests will
be under `/precompile/contracts/helloworld/config_test.go`. There are mainly 2 functions we need
to test: `Verify` and `Equal`. `Verify` checks if the precompile is configured correctly. `Equal`
checks if the precompile is equal to another precompile. You can check other `config_test.go` files
in the `/precompile/contracts` directory for examples. For the `HelloWorld` precompile, you can
check the code in [here](https://github.com/ava-labs/subnet-evm/blob/helloworld-official-tutorial-v2/precompile/contracts/helloworld/config_test.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Let's add some unit tests to make sure our precompile is configured correctly. Config tests will
be under `/precompile/contracts/helloworld/config_test.go`. There are mainly 2 functions we need
to test: `Verify` and `Equal`. `Verify` checks if the precompile is configured correctly. `Equal`
checks if the precompile is equal to another precompile. You can check other `config_test.go` files
in the `/precompile/contracts` directory for examples. For the `HelloWorld` precompile, you can
check the code in [here](https://github.com/ava-labs/subnet-evm/blob/helloworld-official-tutorial-v2/precompile/contracts/helloworld/config_test.go)
Let's add some unit tests to ensure our precompile is configured correctly. Config tests will
be under `/precompile/contracts/helloworld/config_test.go`. We need to test mainly two functions: `Verify` and `Equal.` `Verify` checks if the precompile is configured correctly. `Equal`
checks if the precompile is equal to another precompile. For example, you can check other `config_test.go` files
in the `/precompile/contracts` directory. For the `HelloWorld` precompile, you can
check the code [here](https://github.com/ava-labs/subnet-evm/blob/helloworld-official-tutorial-v2/precompile/contracts/helloworld/config_test.go)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a whitespace diff suggestion? I intentionally split those lines to comfort the enforced MD rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the big suggestion but it was mainly for 2 - >two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thanks. changed it.

> Enter [BeforeSuite] TOP-LEVEL - /Users/aaronbuchwald/go/src/github.com/ava-labs/subnet-evm/tests/precompile/precompile_test.go:31 @ 01/27/23 10:33:51.001
INFO [01-27|10:33:51.002] Starting AvalancheGo node wd=/Users/aaronbuchwald/go/src/github.com/ava-labs/subnet-evm
[BeforeSuite]
/Users/avalabs/go/src/github.com/ava-labs/subnet-evm/tests/precompile/precompile_test.go:31
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

darioush
darioush previously approved these changes Feb 24, 2023
precompile, the address of the precompile and a configurator. This file is located at
[`./precompile/helloworld/module.go`](https://github.com/ava-labs/subnet-evm/blob/helloworld-official-tutorial-v2/precompile/contracts/helloworld/module.go).

It defines the module for the precompile. The module is used to register the precompile to the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It defines the module for the precompile. The module is used to register the precompile to the
This file defines the module for the precompile. The module is used to register the precompile to the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

// This function is called by the EVM once per precompile contract activation.
// You can use this function to set up your precompile contract's initial state,
// by using the [cfg] config and [state] stateDB.
// Configure configures [state] with the initial configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a duplicate comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep removed it, thanks

@@ -1128,31 +1412,33 @@ Compilation finished successfully

ExampleHelloWorld
Contract deployed to: 0x52C84043CD9c865236f11d9Fc9F56aa003c1f922
✓ should getHello properly
✓ should setGreeting and getHello (4103ms)
✓ should getHello properly
Copy link
Contributor

Choose a reason for hiding this comment

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

wspace fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it was a tab, fixed it.

@yulin-dong yulin-dong merged commit 7d9aa81 into master Feb 25, 2023
@yulin-dong yulin-dong deleted the precompile-improvements branch February 25, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants