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

Patch feat/expose expt rpc apis #301

Closed
wants to merge 26 commits into from
Closed

Patch feat/expose expt rpc apis #301

wants to merge 26 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 13, 2023

Addressing code reviews for #285

@ghost
Copy link
Author

ghost commented Sep 13, 2023

Reviews addressed
cc @ChaoticTempest

@ghost ghost marked this pull request as ready for review September 13, 2023 10:54
@ghost
Copy link
Author

ghost commented Sep 25, 2023

@frol This is ready to be reviewed.

Comment on lines 12 to 18
let outcome = contract
_ = contract
.call("set_status")
.args_json(json!({
"message": "hello_world",
}))
.transact()
.await?;
Copy link
Member

Choose a reason for hiding this comment

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

ideally should be using the result of the transaction. Best to add an into_result()? at the end to show best practices

Comment on lines 12 to 17
let outcome = contract
_ = contract
.call("set_status")
.args_json(json!({
"message": "hello_world",
}))
.transact()
.await?;
Copy link
Member

Choose a reason for hiding this comment

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

same here

) -> Result<ProtocolConfigView> {
self.client().protocol_config(block_reference).await
}
pub async fn receipt(&self, ids: &[CryptoHash]) -> Result<ReceiptView> {
Copy link
Member

Choose a reason for hiding this comment

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

why is there a list of IDs passed when only the last one gets used via ids.last()?

StateChangesRequestView::ContractCodeChanges {
account_ids: vec![contract.id().clone()],
}
let state_changes = StateChangesRequestView::ContractCodeChanges {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like how we're utilizing *View structs to commit requests into the network. How about we create our own non-view ones, and that way we don't expose nearcore related dependencies too

Copy link
Author

Choose a reason for hiding this comment

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

Checking on that

Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

Everything else besides the two point I had looks good to me

workspaces/src/worker/impls.rs Show resolved Hide resolved
block_reference: BlockReference,
) -> Result<RpcStateChangesInBlockByTypeResponse> {
self.client().changes_in_block(block_reference).await
pub fn changes(&self, account_ids: Vec<AccountId>) -> Query<'_, StateChanges> {
Copy link
Member

Choose a reason for hiding this comment

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

The inputs to this should just be a slice instead of a Vec as to make it more ergonomic for users to make use of

Suggested change
pub fn changes(&self, account_ids: Vec<AccountId>) -> Query<'_, StateChanges> {
pub fn changes(&self, account_ids: &[AccountId]) -> Query<'_, StateChanges> {

@ghost ghost closed this by deleting the head repository Jul 9, 2024
This pull request was closed.
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