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

Implement account.viewState which allows querying contract state directly #452

Merged
merged 10 commits into from
Nov 20, 2020

Conversation

vgrichina
Copy link
Contributor

No description provided.

@vgrichina vgrichina requested review from chadoh and volovyks November 18, 2020 22:59
@vgrichina vgrichina marked this pull request as ready for review November 18, 2020 23:00
@vgrichina vgrichina changed the title Prototype account.viewState Implement account.viewState which allows querying contract state directly Nov 18, 2020
Copy link
Contributor

@chadoh chadoh 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 this! Excited to see it get in. I'd like to see some changes related to documentation & devx before it gets merged, but nothing that should take long. See below.

const { values } = await this.connection.provider.query({
request_type: 'view_state',
block_id: blockId,
finality: blockId ? undefined : finality || 'optimistic',
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a pretty tricky interplay between blockId & finality here that I'd like to see documented more explicitly. Can we add a code comment above viewState to get picked up by typedoc, as well as throwing an error message when you pass a nonsensical combination of the two?

Copy link
Contributor Author

@vgrichina vgrichina Nov 20, 2020

Choose a reason for hiding this comment

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

yeah, I forgot about the doc here, will add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as well as throwing an error message when you pass a nonsensical combination of the two?

Types kinda describe it. We aren't really checking types during runtime in most other places. TBH this is something I really don't understand about TypeScript. Like doing this whole effort with types and then not having easy way to check in runtime automatically 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created issue to track #457

src/account.ts Outdated
block_id: blockId,
finality: blockId ? undefined : finality || 'optimistic',
account_id: this.accountId,
prefix_base64: Buffer.from(prefix,).toString('base64')
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with the comma in (prefix,)?

src/account.ts Outdated
prefix_base64: Buffer.from(prefix,).toString('base64')
});

return values.map(({key, value}) => ({ key: Buffer.from(key, 'base64'), value: Buffer.from(value, 'base64')}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please break up this long line

@@ -68,13 +68,17 @@ export class JsonRpcProvider extends Provider {

/**
* Query the RPC as [shown in the docs](https://docs.nearprotocol.com/docs/interaction/rpc#query)
* @param path Path parameter for the RPC (ex. "contract/my_token")
* @param data Data parameter (ex. "", "AQ4", or whatever is needed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add documentation to describe the new interface & behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chadoh what inft do you think is going to be useful outside of link to corresponding RPC docs?

If I added a doc in style of the docs we had before it would be something like:

@param params parameters for the RPC

which I don't think is adding value

@@ -241,6 +242,7 @@ export abstract class Provider {

abstract async sendTransaction(signedTransaction: SignedTransaction): Promise<FinalExecutionOutcome>;
abstract async txStatus(txHash: Uint8Array, accountId: string): Promise<FinalExecutionOutcome>;
abstract async query(params: object): Promise<any>;
abstract async query(path: string, data: string): Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to have both versions of query here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes to avoid breaking backwards compatibility

@@ -26,6 +26,7 @@ type BlockHash = string;
type BlockHeight = number;
export type BlockId = BlockHash | BlockHeight;

// TODO: Remove near-final?
Copy link
Contributor

Choose a reason for hiding this comment

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

near-final used to be in the docs, but I don't see it anymore. Maybe this was removed. Who can we ask?

Seems like we could make a separate PR that just removes this option, and request review from people who would know enough to approve or reject the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, that's why I just added TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this is secret feature 🤷‍♂️

@vgrichina
Copy link
Contributor Author

Added docs and tests, etc @chadoh
Going to merge

@vgrichina vgrichina merged commit 67400fa into master Nov 20, 2020
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