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

[NayNay] Only substrate #329

Merged
merged 5 commits into from
Dec 19, 2024
Merged

Conversation

rh0delta
Copy link
Contributor

Related Issue(s)

Proposed Changes

  • updated trasnfer to only need substrate rather than the entire entropy instance

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed

Screenshots (if applicable)

Additional Context

Checklist

  • I have performed a self-review of my code.
  • I have added tests.
  • I have commented my code.
  • I have included a CHANGELOG.md entry.
  • I have updated documentation in github.com:entropyxyz/entropy-docs, where necessary.

- updated trasnfer to only need substrate rather than the entire entropy instance
mixmix
mixmix previously requested changes Dec 17, 2024
Copy link
Contributor

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

@rh0delta can you take this PR a bit further and show what it looks like to remove entropy completely from interatction and command. Right now we still have loadEntropyCLI or loadEntropyTUI happening upstream and the idea I had was that instantiatioin is where errors are handled etc. I want to see the value complete and that it works and we really can get away from loading entropy

@mixmix
Copy link
Contributor

mixmix commented Dec 17, 2024

I shoulda said this seems real promising as a path, nice observation!

@rh0delta
Copy link
Contributor Author

@rh0delta can you take this PR a bit further and show what it looks like to remove entropy completely from interatction and command. Right now we still have loadEntropyCLI or loadEntropyTUI happening upstream and the idea I had was that instantiatioin is where errors are handled etc. I want to see the value complete and that it works and we really can get away from loading entropy

the idea here was to remove the instantiation of entropy where we can. however, transfer still requires some data coming from the entropy instance, i.e the signer pair coming from the keyring, so we are unable to completely remove the load of entropy upstream. The most that could be done currently, is the removal of the need for the entropy instance in the class itself.

@rh0delta
Copy link
Contributor Author

@mixmix may have found a way to completely remove entropy instance from transfer, both in TUI and CLI. please give it a re-review.

Copy link
Collaborator

@frankiebee frankiebee left a comment

Choose a reason for hiding this comment

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

This looks good to me thanks!

@frankiebee frankiebee merged commit b5089be into naynay/any-balance Dec 19, 2024
2 checks passed
@frankiebee frankiebee deleted the naynay/substrate-overload branch December 19, 2024 17:53
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary instantiations of entropy instance when only substrate is used
3 participants