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

Env usage localized in api.js #73

Merged
merged 6 commits into from
Jun 2, 2022
Merged

Env usage localized in api.js #73

merged 6 commits into from
Jun 2, 2022

Conversation

volovyks
Copy link
Collaborator

@volovyks volovyks commented Jun 1, 2022

  • Env usage localized in api.js
  • Stale and trivial low-level examples deleted
  • Low-level nft and counter examples are rewritten (@ailisp do you think we should keep them? If so, I can write tests)
  • Unit test scripts fixed (we should get updated SDK code on each run)

@volovyks volovyks changed the title Env Env usage localized in api.js Jun 1, 2022
@volovyks volovyks marked this pull request as ready for review June 1, 2022 13:45
@volovyks volovyks requested a review from ailisp as a code owner June 1, 2022 13:45
near.jsvmStorageWrite(TOTAL_SUPPLY, tokenId.toString())
near.jsvmStorageWrite('token_to_owner_' + tokenId, ownerId)
near.log(`Minted NFT ${tokenId} to ${ownerId}`)
// env.jsvm_value_return(tokenId.toString()) //TODO: Not implemented. Should we add it?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ailisp I'm not sure what should be done with this function call. The documentation says that it is used together with env.jsvm_call(...args), but we do not have it here.

Also, something I have noticed:

export function jsvmCall(contractName, method, args) {
    env.jsvm_call(contractName, method, JSON.stringify(args), 0)
    return JSON.parse(env.read_register(0) || 'null')
}

In this function we are using env.read_register(...args), not env.jsvm_value_return().

Copy link
Member

Choose a reason for hiding this comment

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

@ailisp I'm not sure what should be done with this function call. The documentation says that it is used together with env.jsvm_call(...args), but we do not have it here.

My bad on describing the behavior. jsvm_value_return does: 1) store the return in a C static variable, so if this were called by another contract, that contract can retrieve this info. 2) plus the normal host function value_return.

This contract doesn't call other contract so there's no jsvm_call, but this contract could be called by another contract. Therefore, for any JS contract, jsvm_value_return should always be used instead of value_return.

In this function we are using env.read_register(...args), not env.jsvm_value_return().

env.jsvm_call saves the the return (if there were jsvm_value_return in the call) in both a C static variable and the given register:

// this is the env.jsvm_call
static JSValue near_jsvm_call(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv) {
  // ...
  jsvm_call(contract_name_len, (char *)contract_name_ptr, method_name_len, (char *)method_name_ptr, arguments_len, (char *)arguments_ptr, true); // inside jsvm_call, if there's a jsvm_value_return, it saves call return to a static variable and also call host's value_return
  jsvm_call_returns(&call_ret, &ret_len, GET); // get the return from that static variable
  write_register(register_id, ret_len, (uint64_t)call_ret); // save to given register
  // ...
}

If we just want to return the value to runtime, jsvm_value_return(value) would do that. If we want to further process this value, we need to read it from register (e.g. in jsvmCall)

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Great improvement to usability! We can be not care of low level APIs now and I'm going to replace them by near.* APIs in docs.

@ailisp
Copy link
Member

ailisp commented Jun 2, 2022

Low-level nft and counter examples are rewritten (@ailisp do you think we should keep them? If so, I can write tests)

Rethink on this, now I prefer to drop them, or move them from examples to tests. Our low level rust contracts, are tests for the (nearcore) runtime, and not part of near-sdk-rs. If we have it here, we need some documentation describing env.* APIs and tech details on it. But we already decide to drop env.* from public APIs. They are more fit to near-sdk-js contributors than contract developers.

Whether we choose to drop or place some low level contracts as tests, nft and counter are not good test candidates that can be dropped now. What do you think?

@volovyks volovyks merged commit f03eb1a into master Jun 2, 2022
@volovyks volovyks deleted the env branch June 7, 2022 12:04
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