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

fix #447 Add rpc endpoint for AutomationPrice #458

Merged
merged 2 commits into from
Oct 25, 2023
Merged

Conversation

v9n
Copy link
Member

@v9n v9n commented Oct 24, 2023

Note: I still had some compilation issue with the runtime/oak. it looks like runtime/oak has missing some code around automation price pallet setup at first place.

Eventually we will need to find a way to consolidate the setup between the runtime env and reduce amount of boiler plate code.

Screenshot 2023-10-24 at 9 15 11 AM

@v9n v9n changed the base branch from master to fee-handle-for-automation-price October 24, 2023 16:21
@v9n v9n force-pushed the add-rpc-endpoint-for-fee branch 2 times, most recently from 95222ae to 68d10e8 Compare October 24, 2023 16:25
@v9n v9n changed the title Add rpc endpoint for fee fix #447 Add rpc endpoint for AutomationPrice Oct 24, 2023
@v9n v9n requested a review from chrisli30 October 24, 2023 17:38
Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

Code looks good to me 👌

@v9n v9n force-pushed the fee-handle-for-automation-price branch from cdc9cc0 to 369b795 Compare October 24, 2023 19:01
Base automatically changed from fee-handle-for-automation-price to master October 24, 2023 20:39
@v9n v9n force-pushed the add-rpc-endpoint-for-fee branch 3 times, most recently from 4a6001e to 46491a6 Compare October 25, 2023 16:01
Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

The code looks good.

@@ -24,6 +24,10 @@ include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs"));

use codec::{Decode, Encode, MaxEncodedLen};
use hex_literal::hex;

// TODO: The FeeDetails RPC is very similar between time/price, it maybe worth to extract them out
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I recorded a Jira ticket for this TODO.

#[cfg(not(feature = "dev-queue"))]
ensure_root(origin)?;
// #[cfg(not(feature = "dev-queue"))]
// ensure_root(origin)?;
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this line is not really needed, because we always have sudo access with dev-queue in local environment.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah this is bad merge. we do need this. it wasn't working so i had to comment it out for test. but after I fixed in this #459 PR then the flag will be pass to this pallet.

Copy link
Member

Choose a reason for hiding this comment

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

okay, yeah I remember commenting on this line 😅

@v9n v9n merged commit 72fda9f into master Oct 25, 2023
3 checks passed
@v9n v9n deleted the add-rpc-endpoint-for-fee branch October 25, 2023 21:39
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.

2 participants