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] Balance for any address & transfer (using only substrate api) #315

Merged
merged 26 commits into from
Jan 8, 2025

Conversation

rh0delta
Copy link
Contributor

@rh0delta rh0delta commented Nov 22, 2024

Related Issue(s)

Proposed Changes

  • updated balance for prg cli to be able to retrieve balance for any address
  • using newly exposed utility method to getSubstrate from sdk

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 balance for prg cli to be able to retrieve balance for any address
- using newly exposed utility method to getSubstrate from sdk
@rh0delta rh0delta added the documentation Improvements or additions to documentation label Nov 22, 2024
@rh0delta rh0delta added this to the Function Completeness milestone Nov 22, 2024
@rh0delta rh0delta linked an issue Nov 22, 2024 that may be closed by this pull request
@rh0delta
Copy link
Contributor Author

Build's will continue to fail for this PR until the addition to the sdk is released.

Re: https://github.com/entropyxyz/sdk/pull/435/files

@mixmix
Copy link
Contributor

mixmix commented Nov 25, 2024

@mixmix reviewing this
@frankiebee working on SDK thing for this

@mixmix mixmix marked this pull request as draft November 25, 2024 23:23
@rh0delta rh0delta removed the blocked label Dec 2, 2024
@rh0delta rh0delta marked this pull request as ready for review December 2, 2024 22:57
package.json Show resolved Hide resolved
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.

Excited for this. Needs more tests to prove it works, and needs to not regress the API

tests/balance.test.ts Outdated Show resolved Hide resolved
src/balance/command.ts Outdated Show resolved Hide resolved
@rh0delta rh0delta requested a review from mixmix December 4, 2024 19:42
@mixmix
Copy link
Contributor

mixmix commented Dec 10, 2024

merge conflicts now here too @rh0delta

@mixmix
Copy link
Contributor

mixmix commented Dec 12, 2024

Found some bugs that I fixed in this PR: #327

@mixmix
Copy link
Contributor

mixmix commented Dec 12, 2024

Found that when I have a newly fauceted account this function doesn't work:

➜  cli git:(mixmix/any-balance) entropy balance --all | jq
[
  {
    "account": "5HNMFB1fELqSAEMkRtUMA6ACs1jD693h16Gt8ixvBS6csJRq",
    "balance": 2,
    "symbol": "BITS"
  },
  {
    "account": "5E4DpqmtnxBA6WbExNvFnJv2Hw5BsUpk8KnAVZLbBMWZVTXb",
    "balance": 0,
    "symbol": "BITS"
  }
]
➜  cli git:(mixmix/any-balance) entropy balance 5HNMFB1fELqSAEMkRtUMA6ACs1jD693h16Gt8ixvBS6csJRq | jq
Provided [account=5HNMFB1fELqSAEMkRtUMA6ACs1jD693h16Gt8ixvBS6csJRq] is not a valid substrate address

@mixmix
Copy link
Contributor

mixmix commented Dec 12, 2024

yeah have tried with a registered account and still getting that error "not a valid substrate address"

@mixmix
Copy link
Contributor

mixmix commented Dec 12, 2024

ok fixed it in that PR : 619ff78

@rh0delta I've seen us slip with if/ else + boolean logic a couple times now. I didn't spot it either till I did some closer testing. I think we need to EITHER write tests over such things, OR try to write much simpler (and more verbose) if/ else if/ else logic. Have a look at the solution in that linked commit. It's more nesting, but more clear (to me at least)

mixmix and others added 4 commits December 13, 2024 15:19
* fix getTokenDetail calls, fix faucet sendMoney...

* ensure load-entropy works with default account (selectedAccount)

* ensure register works with default account (selectedAccount)

* fix transfer relying on opts.account

* fix balance account-checking logic
- updated trasnfer to only need substrate rather than the entire entropy instance
@mixmix
Copy link
Contributor

mixmix commented Dec 16, 2024

Mix to re-review

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.

I like this, it's really interesting seeing how little we might need the full SDK... also begs the question "what is it even used for", "what is our policy for when we use SDK and when we use raw substrate?"

I'd like to see us not go to full class methods - I think keeping some of the pattern of EntropyBase but making a SubstrateBase class that we inherit from will keep the patterns of the repo more coherent and give use support for the logging and other things that we need, tidy up need to pass around substrate etc.

Have left more notes and thinking as I went with more detail.
Nice work!

src/account/utils.ts Outdated Show resolved Hide resolved
src/balance/main.ts Outdated Show resolved Hide resolved
src/common/substrate-utils.ts Outdated Show resolved Hide resolved
@rh0delta
Copy link
Contributor Author

@mixmix mistakenly made most of the pr updates in this PR #329, so we can merge that into this one once reviewed

@frankiebee frankiebee changed the title [NayNay] Balance for any address [NayNay] Balance for any address & transfer (using only substrate api) Dec 19, 2024
@rh0delta rh0delta dismissed mixmix’s stale review January 8, 2025 18:44

changes made, mix ooo, need a review from frankie

@frankiebee
Copy link
Collaborator

I like this, it's really interesting seeing how little we might need the full SDK... also begs the question "what is it even used for", "what is our policy for when we use SDK and when we use raw substrate?"

you probably asking this because you have'nt needed to deal with signing. The sdk is meant to make signing and the format checking of registering with programs simple.

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.

Looks good to me thanks for the work!

@frankiebee frankiebee merged commit 010f672 into dev Jan 8, 2025
2 checks passed
@frankiebee frankiebee deleted the naynay/any-balance branch January 8, 2025 21:29
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

entropy balance <address> should take any address
3 participants