Skip to content

simplify extends for the account class #1021

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

Merged
merged 6 commits into from
Mar 19, 2024
Merged

simplify extends for the account class #1021

merged 6 commits into from
Mar 19, 2024

Conversation

0xknwn
Copy link
Contributor

@0xknwn 0xknwn commented Mar 18, 2024

Motivation and Resolution

I am working with Starknet abstract account. To support our work, I am extending the starknet.js Account class to add some additional behaviours on the application side. I am not changing the way the transaction version is used internally and would still need to implement the account deployment.

You have defined 3 private methods in the class which makes sense to not expose to the outside. However in the context of extending that class we would prefer to access those 3 methods instead of rewriting them. Obviously, this is your decision to make. For that particular scenario, I think having those 3 private methods turned into protected makes more sense.

RPC version (if applicable)

  • not applicable

Usage related changes

  • It will not affect any user that is not extending the account class (i.e. most developers)
  • It will simplify the ability to extend the Account

Development related changes

  • We did not add any test as the protected functions can still not be accessed from the outside and the code can easily be copied/pasted into a new private function anyway.
  • If you think something should be done to enhanced my request, let me know.

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

penovicp and others added 6 commits March 14, 2024 16:05
# [6.5.0](v6.4.2...v6.5.0) (2024-03-14)

### Bug Fixes

* adjust max amount bound calculation for RPC v0.7.0 ([dd34cdb](dd34cdb))

### Features

* make fee margins configurable ([cedd984](cedd984))
* docs: Add docker approach guide for dev setup

* docs: Add more detailed guide steps

* docs: add specific docker command via guide

* chore: extract devnet detector & env helper

* chore: throw an error if devnet is not set

* docs: remove devnet py detection logging

* docs: writing style improvement

---------

Co-authored-by: Luka Saric <[email protected]>
Copy link
Member

@tabaktoni tabaktoni left a comment

Choose a reason for hiding this comment

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

LGTM!

@tabaktoni
Copy link
Member

If you can please do share an example of your AA implementation it could be a valuable resource for others.

@tabaktoni tabaktoni changed the base branch from develop to next-version March 19, 2024 16:36
@tabaktoni tabaktoni merged commit b4ea22a into starknet-io:next-version Mar 19, 2024
@tabaktoni tabaktoni mentioned this pull request Mar 19, 2024
6 tasks
@0xknwn
Copy link
Contributor Author

0xknwn commented Mar 19, 2024

If you can please do share an example of your AA implementation it could be a valuable resource for others.

I will for sure! Both the Cairo and the Starknet.js implementation. I just need some time to get it clean

Also thank to the 2 of you @tabaktoni and @ivpavici for this lightning merge!

Copy link
Contributor

🎉 This PR is included in version 6.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@0xknwn
Copy link
Contributor Author

0xknwn commented Mar 20, 2024

🎉 This PR is included in version 6.6.0 🎉

Actually, no it is not. Can you double-check? It could be because of the error in the CI. Thank you

@ivpavici
Copy link
Collaborator

ivpavici commented Mar 20, 2024

🎉 This PR is included in version 6.6.0 🎉

Actually, no it is not. Can you double-check? It could be because of the error in the CI. Thank you

yeah the github-actions post here is wrong, it will be included in the next version! 🙏
@penovicp can you check please when you find some time?

@penovicp
Copy link
Collaborator

The notification does look like a bug. My guess is that it might stem from an overlap arising from the next-version branch name, that when identifying PRs to notify the plugin expands the set with PRs that have been merged into branches that have themselves been merged into the configured release branch without or with faulty verification of the included commits. Essentially my presumption is the following sequence: next-version merged to develop; this PR merged to next-version; manual release; faulty notification for all merges into next-version and not just from the first step.

I haven't found an existing corresponding issue and would need to do a deep dive into the release plugin code to corroborate my assumption.

Copy link
Contributor

github-actions bot commented Jul 3, 2024

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants