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

dev: update Madara genesis loading #484

Closed
wants to merge 5 commits into from

Conversation

ftupas
Copy link
Contributor

@ftupas ftupas commented Aug 25, 2023

Time spent on this PR: 0.2

Depends on: keep-starknet-strange/madara#1018
Resolves: #477

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Testing

What is the new behavior?

Uses the official Madara binary and update the genesis json to the latest format

Does this introduce a breaking change?

  • Yes
  • No

Added changes to CHANGELOG.md

  • Yes
  • No

Copy link
Contributor Author

@ftupas ftupas left a comment

Choose a reason for hiding this comment

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

Some TODOs for me in this pr

@@ -1,6 +1,6 @@
# TODO: Use official madara image when https://github.com/keep-starknet-strange/madara/issues/982 is closed
# FROM ghcr.io/keep-starknet-strange/madara:v0.1.0.experimental.3 as madara
FROM fredtupas/madara as madara
FROM fredtupas/madara-fixed as madara
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: will update pr to official image once this pr in Madara is merged keep-starknet-strange/madara#1018


# Download Madara genesis config to source path
RUN svn export https://github.com/keep-starknet-strange/madara/trunk/configs ${MADARA_CONFIG_PATH}
RUN svn export https://github.com/d-roak/madara/branches/feat/configs-index/configs/genesis-assets ${MADARA_GENESIS_ASSETS_PATH}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Update to Madara's main branch

@@ -8,6 +8,7 @@ export PROXY_ACCOUNT_CLASS_HASH=$(starkli class-hash ${MADARA_PATH}/cairo-contra
--rpc-external \
--rpc-methods=unsafe \
--rpc-cors=all \
--disable-madara-configs \
Copy link
Contributor Author

@ftupas ftupas Aug 25, 2023

Choose a reason for hiding this comment

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

Since we're copying all the genesis assets into the local Madara path, we need to add this flag to use them

@ftupas ftupas force-pushed the dev/use-official-madara-hive branch from 7f4040c to 8a47a42 Compare August 25, 2023 12:20
@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Patch coverage: 83.25% and project coverage change: +49.21% 🎉

Comparison is base (71506a8) 23.17% compared to head (ba37efa) 72.38%.
Report is 191 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #484       +/-   ##
===========================================
+ Coverage   23.17%   72.38%   +49.21%     
===========================================
  Files           9       44       +35     
  Lines        1247     3998     +2751     
===========================================
+ Hits          289     2894     +2605     
- Misses        958     1104      +146     
Files Changed Coverage Δ
crates/core/src/mock/serde.rs 0.00% <0.00%> (ø)
crates/core/src/models/transaction.rs 93.63% <ø> (ø)
crates/core/src/models/transaction_receipt.rs 97.02% <ø> (ø)
crates/core/src/test_utils/bin/dump-katana.rs 0.00% <ø> (ø)
crates/core/src/test_utils/deploy_helpers.rs 32.44% <ø> (ø)
crates/core/src/test_utils/execution_helpers.rs 100.00% <ø> (ø)
crates/core/src/test_utils/fixtures.rs 100.00% <ø> (ø)
crates/eth-rpc/src/api/alchemy_api.rs 100.00% <ø> (ø)
crates/eth-rpc/src/api/eth_api.rs 100.00% <ø> (ø)
crates/eth-rpc/src/api/net_api.rs 100.00% <ø> (ø)
... and 34 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -8,6 +8,7 @@ export PROXY_ACCOUNT_CLASS_HASH=$(starkli class-hash ${MADARA_PATH}/cairo-contra
--rpc-external \
--rpc-methods=unsafe \
Copy link
Contributor

Choose a reason for hiding this comment

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

u can remove --rpc-externaland --rpc-methods=unsafe because they will be enabled when u set the --dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks will remove this!

@@ -8,6 +8,7 @@ export PROXY_ACCOUNT_CLASS_HASH=$(starkli class-hash ${MADARA_PATH}/cairo-contra
--rpc-external \
--rpc-methods=unsafe \
--rpc-cors=all \
--disable-madara-configs \
--tmp \
Copy link
Contributor

Choose a reason for hiding this comment

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

why are u setting a tmp path for the db? since it's a docker container, u can just let it go to the default madara-path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove this, had to add this flag before in the fork because it was initializing but not including the Kakarot contracts for some reason and it worked when we added it

Copy link
Contributor Author

@ftupas ftupas left a comment

Choose a reason for hiding this comment

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

Thanks for the comments @d-roak !

@@ -8,6 +8,7 @@ export PROXY_ACCOUNT_CLASS_HASH=$(starkli class-hash ${MADARA_PATH}/cairo-contra
--rpc-external \
--rpc-methods=unsafe \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks will remove this!

@@ -8,6 +8,7 @@ export PROXY_ACCOUNT_CLASS_HASH=$(starkli class-hash ${MADARA_PATH}/cairo-contra
--rpc-external \
--rpc-methods=unsafe \
--rpc-cors=all \
--disable-madara-configs \
--tmp \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove this, had to add this flag before in the fork because it was initializing but not including the Kakarot contracts for some reason and it worked when we added it

@ClementWalter
Copy link
Member

hey, what's the blocker for moving this into ready for review?

@ftupas
Copy link
Contributor Author

ftupas commented Aug 29, 2023

hey, what's the blocker for moving this into ready for review?

Ah I was waiting for this pr to be merged but actually I think we can have this ready for review 🙂 I just pulled the latest commits from Oak's pr and tested it locally and confirming it's still working, moving it to ready

image

@ftupas ftupas marked this pull request as ready for review August 29, 2023 17:11
@d-roak
Copy link
Contributor

d-roak commented Aug 29, 2023

That PR is going to take a big refactor soon. Be careful while using it

@ClementWalter
Copy link
Member

@d-roak so you suggest that we don't merge yet?

@Eikix
Copy link
Member

Eikix commented Sep 5, 2023

That PR is going to take a big refactor soon. Be careful while using it

Can I get the context for this refactor, to have more clarity on our steps

@d-roak
Copy link
Contributor

d-roak commented Sep 5, 2023

Sry for the delay!

@ClementWalter U can merge it, but instead of fixing the branch, you should fix a specific commit within the branch. Once it's merge on main, I can help you refactor whatever is needed on your side

@Eikix We are changing the way that we run/setup madara. So we are going to have:
madara setup
madara run --dev
In your specific use case, you might want to pass some flags to madara setup, or, set it manually (i believe that's what u are doing atm)

@ClementWalter ClementWalter marked this pull request as draft September 12, 2023 14:15
@greged93
Copy link
Collaborator

greged93 commented Nov 6, 2023

@ClementWalter is it ok to close this for you? I will make one high level issue to rework everything linked with dumping

@d-roak
Copy link
Contributor

d-roak commented Nov 7, 2023

@greged93 lmk if you want me to review anything madara wise

@greged93
Copy link
Collaborator

greged93 commented Nov 7, 2023

@d-roak we're doing a big clean of this repo, will rework the madara part later on and let you know if we need some help

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.

dev: use official Madara image/binary
5 participants