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.

Sign up for GitHub

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

Jump to bottom

Better get balance #433

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Better get balance #433

wants to merge 20 commits into from
+177 −37

Conversation

slavastartsev
Copy link
Contributor

@slavastartsev slavastartsev commented Dec 12, 2024 •
edited
Loading

  • for each unconfirmed tx, check there are no inscriptions:

    bob/sdk/src/inscription.ts

    Line 188 in 743d020

    return parseInscriptions(tx);
  • for each input to that tx do step (1) if its also unconfirmed else check via the Ord API as we do now

Explanation: build a tree structure

  • for each output returned by esplora becomes root TreeNode
  • inputs to that tx are it's children
  • leaf nodes either confirmed outputs or outputs containing ordinals (flagged with cardinal: false).

If output has all leaf nodes with cardinal: true -- confirmed (not containing ordinals) then output amount could be spent.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 1cdf87b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@vercel Vercel
Copy link

vercel bot commented Dec 12, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
bob ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2024 1:49pm

@slavastartsev slavastartsev marked this pull request as ready for review December 12, 2024 14:33
slavastartsev
slavastartsev commented Dec 12, 2024
View reviewed changes
sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
slavastartsev
slavastartsev commented Dec 12, 2024
View reviewed changes
sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
gregdhill
gregdhill reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@gregdhill gregdhill left a comment

Choose a reason for hiding this comment

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

I also see similar logic changes in at least 2-3 parts of utxo.ts is there anyway to share logic?

sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
sdk/test/utxo.test.ts Show resolved Hide resolved
slavastartsev
slavastartsev commented Dec 13, 2024
View reviewed changes
sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
slavastartsev
slavastartsev commented Dec 13, 2024
View reviewed changes
sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
gregdhill
gregdhill requested changes Dec 19, 2024
View reviewed changes
sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
@slavastartsev slavastartsev force-pushed the feat/better-get-balance branch from b5922db to 09d97ff Compare December 19, 2024 16:47
@slavastartsev slavastartsev force-pushed the feat/better-get-balance branch from 09d97ff to d92d198 Compare December 19, 2024 16:56
sander2
sander2 reviewed Dec 19, 2024
View reviewed changes
Copy link
Contributor

@sander2 sander2 left a comment

Choose a reason for hiding this comment

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

Would it be possible to add some tests?

sdk/src/wallet/utxo.ts Outdated
// if confirmed check if it contains ordinals
childNode.val.cardinal = cardinalOutputsSet.has(OutPoint.toString(childNode.val));
} else {
const response = await ordinalsClient.getInscriptionsFromOutPoint(childNode.val);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the utxo is unconfirmed? Would the next line panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it won't

curl -s -H "Accept: application/json" "https://ordinals-mainnet.gobob.xyz/output/9cd84b0d89695c732f42906e755894319f6afa1aa69fef0b9a0bc5974eed8ef9:2"

sdk/src/wallet/utxo.ts Outdated Show resolved Hide resolved
@slavastartsev
Copy link
Contributor Author

Would it be possible to add some tests?

tests will be added once we agree on the final solution

@slavastartsev slavastartsev force-pushed the feat/better-get-balance branch from a4d0683 to 43f89ac Compare December 20, 2024 13:42
@slavastartsev slavastartsev force-pushed the feat/better-get-balance branch from 43f89ac to 5ae9e5b Compare December 20, 2024 13:46
@slavastartsev slavastartsev force-pushed the feat/better-get-balance branch from 5ae9e5b to 1cdf87b Compare December 20, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sander2 sander2 sander2 left review comments

@gregdhill gregdhill gregdhill requested changes

@tomjeatt tomjeatt Awaiting requested review from tomjeatt

@danielsimao danielsimao Awaiting requested review from danielsimao

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@slavastartsev @sander2 @gregdhill