-
Notifications
You must be signed in to change notification settings - Fork 88
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
Refactor account balance for support balance API and add some test case #774
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
After refactor, encounter a deadlock in RPC Server. Try to solve RPC server hang caused by futures::executor::block_on when impl MoveFunctionCall for the RPC Server. |
pub struct AnnotatedCoinView { | ||
type_: StructTagView, | ||
struct_type: StructTagView, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to unify the StructTag
field name.
Sometimes, we use struct_tag
rooch/moveos/moveos-types/src/object.rs
Lines 377 to 381 in a635258
#[derive(Eq, PartialEq, Debug, Clone)] | |
pub struct RawData { | |
pub struct_tag: StructTag, | |
pub value: Vec<u8>, | |
} |
Sometimes, we use type_
rooch/crates/rooch-rpc-api/src/jsonrpc_types/move_types.rs
Lines 49 to 55 in a635258
pub struct AnnotatedMoveStructView { | |
pub abilities: u8, | |
#[serde(rename = "type")] | |
pub type_: StructTagView, | |
//We use BTreeMap to Replace Vec to make the output more readable | |
pub value: BTreeMap<Identifier, AnnotatedMoveValueView>, | |
} |
But in the move-language/move repo, AnnotatedMoveStruct use type_
.
pub struct AnnotatedMoveStruct {
pub abilities: AbilitySet,
pub type_: StructTag,
pub value: Vec<(Identifier, AnnotatedMoveValue)>,
}
What do you think about this? @wow-sven @stevenlaw123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unify to type_
and rename via serde?
#[serde(rename = "type")]
pub type_: StructTagView,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already unify to type_
// self.rpc_service.execute_view_function(function_call).await | ||
// })?; | ||
|
||
let function_result = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not use futures::executor::block_on
here, we need to get the tokio Runtime and use the Runtime block_on. https://tokio.rs/tokio/topics/bridging
} | ||
|
||
lazy_static! { | ||
static ref RUNTIME: Runtime = tokio::runtime::Builder::new_multi_thread() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tokio runtime needs to pass from the main function to the service. This runtime is another runtime, not in the same runtime which starts the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will encounter hung up if only one runtime, when use futures::executor::block_on
.
I tried use tokio::block_up, but also failed when call_function
execute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main function runtime should be built with multi-thread. If this approach works, we can refactor it later.
@@ -239,6 +239,17 @@ module rooch_framework::coin { | |||
} | |||
} | |||
|
|||
/// Return coin info handle | |||
public fun coin_info_handle(ctx: &StorageContext): Option<ObjectID> { | |||
if (account_storage::global_exists<CoinInfos>(ctx, @rooch_framework)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be ensured via the Genesis transaction, so it should always exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pub struct AnnotatedCoinView { | ||
type_: StructTagView, | ||
struct_type: StructTagView, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unify to type_
and rename via serde?
#[serde(rename = "type")]
pub type_: StructTagView,
resolve TODOs in #760
And refactor account balance for support Rooch balance API.