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

Naj 44 make sign and send transaction functionality public #1002

Merged

Conversation

esaminu
Copy link
Contributor

@esaminu esaminu commented Sep 30, 2022

Motivation

NAJ-44

Description

Make Account.signAndSendTransaction public so transactions can be sent directly from Account instances

Checklist

  • Read the contributing guidelines
  • Commit messages follow the conventional commits spec
  • Performed a self-review of the PR
  • Added automated tests
  • Manually tested the change

@changeset-bot
Copy link

changeset-bot bot commented Sep 30, 2022

🦋 Changeset detected

Latest commit: 330b2db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
near-api-js Major
@near-js/cookbook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@andy-haynes
Copy link
Collaborator

andy-haynes commented Sep 30, 2022

NAJ-44 mentions

We cannot just remove the protected accessor because that would be a breaking change (every implementation of Account would have to update).

Is there more context to this on the original issue (I can't find the link)? I'm not seeing how this is a breaking change, assuming that "implementation of Account" refers to creating a subclass of it.

@esaminu
Copy link
Contributor Author

esaminu commented Sep 30, 2022

@andy-haynes Yes it appears that's what the ticket is referring to. i.e. if you extend account you'd have to make this method public too as opposed to protected. There's a discussion this here as well: #557 (comment)

This is not an issue imo but we have to bump a major here (updating changeset)

@andy-haynes
Copy link
Collaborator

@andy-haynes Yes it appears that's what the ticket is referring to. i.e. if you extend account you'd have to make this method public too as opposed to protected. There's a discussion this here as well: #557 (comment)

This is not an issue imo but we have to bump a major here (updating changeset)

Sounds good, thanks!

Copy link
Contributor

@hcho112 hcho112 left a comment

Choose a reason for hiding this comment

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

LGTM

@esaminu esaminu merged commit a330763 into master Oct 3, 2022
@esaminu esaminu deleted the NAJ-44-make-sign-and-send-transaction-functionality-public branch October 3, 2022 11:05
@github-actions github-actions bot mentioned this pull request Oct 3, 2022
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.

4 participants