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

Adding access keys to nearlib #918

Merged
merged 2 commits into from Apr 30, 2019

Conversation

Projects
None yet
3 participants
@evgenykuzyakov
Copy link
Collaborator

commented Apr 29, 2019

Adding a way to add access keys in nearlib.
Adding a test to add an access key and call a contract using an access key.
For #687

@@ -1,7 +1,7 @@
{
"name": "nearlib",
"description": "Javascript library to interact with near blockchain",
"version": "0.5.4",
"version": "0.5.5",

This comment has been minimized.

Copy link
@vgrichina

vgrichina Apr 30, 2019

Member

@evgenykuzyakov isn't this change breaking API? Means this should be version 0.6.0, see http://semver.org

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Apr 30, 2019

Author Collaborator

It's non-breaking change, but it does require certain version of nearcore that has proto changes.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Apr 30, 2019

Author Collaborator

But it's not backward compatible, so bumping to 0.6.0.
Not going to publish it until we add the new signer

@@ -1,6 +1,6 @@
const bs58 = require('bs58');

const { CreateAccountTransaction, SignedTransaction } = require('./protos');
const { google, AccessKey, AddKeyTransaction, CreateAccountTransaction, SignedTransaction } = require('./protos');

This comment has been minimized.

Copy link
@janedegtiareva

janedegtiareva Apr 30, 2019

Member

do we need to add the new classes to index.js and browser.js?

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Apr 30, 2019

Author Collaborator

No, since we don't return them to the browser or index

newAccountId,
keyForAccessKey.getPublicKey(),
contractId,
'',

This comment has been minimized.

Copy link
@vgrichina

vgrichina Apr 30, 2019

Member

no idea what are '', '', 0 here without going to check addAccessKey method.

I think it makes sense to either pass parameters as part of options object, or save them to named variables before passing (unless immediately obvious what it is)

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Apr 30, 2019

Author Collaborator

My IDE shows which argument it is for unnamed args. I'll add comments

This comment has been minimized.

Copy link
@vgrichina

vgrichina Apr 30, 2019

Member

@evgenykuzyakov IDE does this, but e.g. right here we don't have this feature available. It's not like most time reading code is spent inside of IDE.

changeMethods: ['setValue'],
sender: newAccountId,
});
const helloResult = await contract.hello({ name: 'trex' });

This comment has been minimized.

Copy link
@vgrichina

vgrichina Apr 30, 2019

Member

looks like contract.hello call isn't necessary for test

also I think at this point it makes sense to extract "test that we can write value and read back" into a helper function (cause it's useful for multiple tests)

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Apr 30, 2019

Author Collaborator

Yeah. I guess view calls are not needed, unless we change a logic.

@evgenykuzyakov evgenykuzyakov merged commit 3faa82b into master Apr 30, 2019

1 check passed

ci/gitlab/add-access-key-to-nearlib Pipeline passed on GitLab
Details

@evgenykuzyakov evgenykuzyakov deleted the add-access-key-to-nearlib branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.