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

Update crafters checklist #28

Merged
merged 4 commits into from
May 23, 2024
Merged

Conversation

SidestreamColdMelon
Copy link
Contributor

Closes #27. A follow up to #18.

  • Improves unclear differentiation between items that are not present and can be skipped and items that are not present, but have to be present
  • Improves unclear differentiation between actionable and categories
  • Improves order of operations, adds few explicit communication responsibilities
  • Improves wording of specific actions

* [ ] Check local env
* EIP1559
* [ ] Run `make estimate` to estimate gas
* [ ] `export ETH_GAS=X` with the output of the command above (e.g. `export ETH_GAS=6_000_000`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Gas estimations might not be 100% accurate:

Suggested change
* [ ] `export ETH_GAS=X` with the output of the command above (e.g. `export ETH_GAS=6_000_000`)
* [ ] `export ETH_GAS=X` with the output of the command above + a safety margin (e.g. `export ETH_GAS=6_000_000`)

* EIP1559
* [ ] Run `make estimate` to estimate gas
* [ ] `export ETH_GAS=X` with the output of the command above (e.g. `export ETH_GAS=6_000_000`)
* [ ] Check current gas price using `seth gas-price` and set `ETH_GAS_PRICE` accordingly (e.g. `50 gwei`)
Copy link
Contributor

@amusingaxl amusingaxl May 14, 2024

Choose a reason for hiding this comment

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

Wouldn't it be better to fetch it from the same source as the priority fee?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be, but the aim of this PR is to just improve the structure or in some cases add/improve rules, but not change them, so I would put this out of scope 🙂

* [ ] Set `deployed_spell` to `address(0)`
* [ ] Set `deployed_spell_created` to `0`
* [ ] Set `deployed_spell_block` to `0`
* [ ] Consider to add `previous_spell` address IF it haven't been executed yet
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
* [ ] Consider to add `previous_spell` address IF it haven't been executed yet
* [ ] IF there are outstanding spells that have not been cast yet, add them to `previous_spells`.

Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

LGTM

@SidestreamColdMelon SidestreamColdMelon merged commit 5436dc2 into master May 23, 2024
2 checks passed
@SidestreamColdMelon SidestreamColdMelon deleted the update-crafter-checklist branch May 23, 2024 08:36
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.

Update crafters checklist to the conditional structure
3 participants