Modify `ord list` output to include the end of each range and the off… #1998

gmart7t2 · 2023-04-03T00:45:17Z

Modify ord list output to include the end of each range and the offset into the output of the start of each range.

Sometimes when sat hunting I will find a sat that I want to isolate and it will be in the 1000th range in its output. I want to use ord wallet send to send the sat to a new address but to do that i need to know its satpoint, which means its txid, vout, and offset.

The txid and vout are obvious. They define the output I am looking at. But to know the offset I have to sum the sizes of the 999 ranges before the sat I am interested in.

I figure it is better to have ord do that summing than to expect the user to do it for themselves., so that's what this change does.

…set into the output of the start of each range.

soenkehahn · 2023-04-04T21:51:07Z

Thanks for the PR! Overall this looks like a useful feature.

Here's a few comments:

Could you add a description of your motivation/use-case for this feature, please? It's easy to imagine use-cases, but it might help review this issue -- especially for people less familiar with all the details of ord -- if we have a very concrete example for when this would be good to have. (I think the first comment aka the issue description is a good place for that.)
Could you make all the checks pass? Currently for example, the tests don't compile.
Could you add some tests for this feature, please? Generally we are aiming for very comprehensive test coverage for ord. Additionally, adding a test may make it much easier for more people to review the PR, since the tests document what is being changed. (And prove that the changes work.)

I'll turn this PR into a draft PR. Please, undraft when you think it's ready for another look.

Thanks again for contributing!

gmart7t2 · 2023-04-09T22:32:56Z

Could you add a description of your motivation/use-case for this feature, please?

Done.

Could you make all the checks pass? Currently for example, the tests don't compile.

I can't reproduce that. Everything seems to be passing. Do you have details of what's failing?

Could you add some tests for this feature, please?

I modified the existing test to expect the new output fields.

I'll turn this PR into a draft PR. Please, undraft when you think it's ready for another look.

Ok.

…e tests.

casey · 2023-04-19T19:25:27Z

@soenkehahn Want to give this another look?

soenkehahn · 2023-04-19T21:45:08Z

I'd love to. This feature needs a --index-sats index, I'm in the process of acquiring one. I felt I couldn't really review this without being able to try it out myself.

soenkehahn

Sorry it took so long to review!

src/subcommand/list.rs

soenkehahn · 2023-04-23T00:19:19Z

Could you make all the checks pass? Currently for example, the tests don't compile.

I can't reproduce that. Everything seems to be passing. Do you have details of what's failing?

Here's the failing log: https://github.com/casey/ord/actions/runs/4591718231/jobs/8108123525. But you fixed it in later commits.

casey

I just wanted to follow up on Sönke's review with my own! Sorry for letting this and other PRs of yours languish.

This looks good, and I think it's close to being mergeable. The only issue that needs to be addressed before merging is that the tuple is getting out of hand. Now that there are two new fields in this tuple, the list function should definitely return Vec<Output> for clarity.

So the new list function signature would be:

fn list(
  outpoint: OutPoint,
  ranges: Vec<(u64, u64)>,
) -> Vec<Output> {
  …
}

@soenkehahn started helping with project maintenance recently, so we're still syncing up on the coding and testing style. For this PR, the current tests are good enough, so you can ignore the other comments for now.

Superseded by Casey's review

raphjaph

Made the changes Casey requested.

* Add contributing section (ordinals#2261) * Implement clean index shutdown to prevent index corruption (with clippy updates for Rust 1.71) (ordinals#2275) * gracefully shutdown index update thread to prevent index corruption * Use `next_back()` instead of `rev().next()` for rust 1.71 --------- Co-authored-by: victorkirov <[email protected]> * Modify `ord list` output to include the end of each range (ordinals#1998) * Don't create default data directory if --index overrides it (ordinals#1991) * Fix docs inconsistency (ordinals#2276) * Fix ordering for reinscriptions and show all reinscriptions for sat (ordinals#2279) * Add satpoint and address to index export (ordinals#2284) * Update bitcoin dependencies (ordinals#2281) * Update redb (ordinals#2294) * Add retry to fetcher (ordinals#2297) * Clean up deploy scripts (ordinals#2298) * Fix justfile recipe (ordinals#2299) * Release 0.8.1 (ordinals#2300) * Add `amount` field to `wallet inscriptions` output. (ordinals#1928) * Fix dust limit for padding in `TransactionBuilder` (ordinals#1929) * Inform user when redb starts in recovery mode (ordinals#2304) * Fix remote RPC wallet commands (ordinals#1766) * Select multiple utxos (ordinals#2303) Co-authored-by: Greg Martin <[email protected]> * feat: add outputs api --------- Co-authored-by: raph <[email protected]> Co-authored-by: victorkirov <[email protected]> Co-authored-by: gmart7t2 <[email protected]> Co-authored-by: ordinally <[email protected]> Co-authored-by: Carlos Alaniz <[email protected]> Co-authored-by: Greg Martin <[email protected]>

)

Modify ord list output to include the end of each range and the off…

6c6d049

…set into the output of the start of each range.

soenkehahn marked this pull request as draft April 4, 2023 21:51

Greg Martin and others added 3 commits April 5, 2023 02:45

Fix tests.

9041567

Format

d612b12

Merge branch 'master' into add-offset-to-list

8189f46

Move offset calculation in list(), and add the expected offsets to th…

6f543ac

…e tests.

gmart7t2 marked this pull request as ready for review April 9, 2023 22:47

gmart7t2 force-pushed the add-offset-to-list branch from f07ef7a to 6f543ac Compare April 14, 2023 22:02

This comment was marked as off-topic.

Sign in to view

soenkehahn previously requested changes Apr 23, 2023

View reviewed changes

src/subcommand/list.rs Outdated Show resolved Hide resolved

src/subcommand/list.rs Show resolved Hide resolved

casey requested review from soenkehahn and casey and removed request for soenkehahn April 25, 2023 19:09

casey requested changes Apr 25, 2023

View reviewed changes

raphjaph added 2 commits July 15, 2023 21:28

Merge branch 'master' into add-offset-to-list

530e582

quick fix

dcc6a6a

raphjaph approved these changes Jul 15, 2023

View reviewed changes

gmart7t2 force-pushed the add-offset-to-list branch from dcc6a6a to 6f543ac Compare July 15, 2023 20:00

casey self-requested a review July 15, 2023 21:00

casey approved these changes Jul 15, 2023

View reviewed changes

raphjaph merged commit 3f2198f into ordinals:master Jul 15, 2023
12 checks passed

Raylin51 pushed a commit to 0xaklabs/ord that referenced this pull request Aug 11, 2023

Modify ord list output to include the end of each range (ordinals#1998

5a0acf2

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify `ord list` output to include the end of each range and the off… #1998

Modify `ord list` output to include the end of each range and the off… #1998

gmart7t2 commented Apr 3, 2023 •

edited

Loading

soenkehahn commented Apr 4, 2023

gmart7t2 commented Apr 9, 2023

casey commented Apr 19, 2023

soenkehahn commented Apr 19, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

soenkehahn left a comment

soenkehahn commented Apr 23, 2023

casey left a comment

raphjaph left a comment

Modify ord list output to include the end of each range and the off… #1998

Modify ord list output to include the end of each range and the off… #1998

Conversation

gmart7t2 commented Apr 3, 2023 • edited Loading

soenkehahn commented Apr 4, 2023

gmart7t2 commented Apr 9, 2023

casey commented Apr 19, 2023

soenkehahn commented Apr 19, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

soenkehahn left a comment

Choose a reason for hiding this comment

soenkehahn commented Apr 23, 2023

casey left a comment

Choose a reason for hiding this comment

raphjaph left a comment

Choose a reason for hiding this comment

Modify `ord list` output to include the end of each range and the off… #1998

Modify `ord list` output to include the end of each range and the off… #1998

gmart7t2 commented Apr 3, 2023 •

edited

Loading