zingolabs / zingolib_original

ZingoLib
MIT License
1 stars 1 forks source link

Fix overloaded tx #12

Closed AloeareV closed 2 years ago

AloeareV commented 2 years ago

This should be clearer. Big change.

zancas commented 2 years ago

I will check this out and see if the tests run.

zancas commented 2 years ago

I know that @dannasessha already raised the issue, but if there're changes to pub vars that could break dependents.

I think that a way to check for this is to build and run all tests in https://github.com/zingolabs/zecwallet-lite against this feature.

If that succeeds, then at least one dependent is not borken!

dannasessha commented 2 years ago

I'm digging into this, the plan is to make two passes.

The first will be to assure that name changes have occurred properly (ie, the code's functioning is unchanged), and the second will be to examine the names themselves (ie, do the names make consistent sense).

I'll also make some notes with cargo tarpaulin here.

dannasessha commented 2 years ago

Tests are passing on both dev and fix_overloaded_tx branches.

Running cargo tarpaulin --avoid-cfg-tarpaulin against... dev shows 63.37% coverage, 3991/6298 lines covered fix_overloaded_tx shows 63.62% coverage, 4051/6367 lines covered

zancas commented 2 years ago

@dannasessha I am signing off on:

  1. cli/src/lib.rs
  2. cli/src/main.rs
  3. lib/proto/compact_formats.proto
  4. lib/proto/service.proto
  5. lib/src/blaze.rs
  6. lib/src/blaze/block_witness_data.rs

    I reviewed them, and think they are internally correct. To merge this code we will need to validate that interfacing projects still interoperate.

    Consumers/Interactors.:

    1. the cli (this repo)
    2. https://github.com/zingolabs/zecwallet-mobile (android and iOS)
    3. https://github.com/zingolabs/zecwallet-lite (the desktop)
    4. https://github.com/zingolabs/lightwalletd (the server)

In the interest of efficiency you should focus on a different part of the diff.

dannasessha commented 2 years ago

@dannasessha I am signing off on:

  1. cli/src/lib.rs
  2. cli/src/main.rs
  3. lib/proto/compact_formats.proto
  4. lib/proto/service.proto
  5. lib/src/blaze.rs
  6. lib/src/blaze/block_witness_data.rs

I reviewed them, and think they are internally correct. To merge this code we will need to validate that interfacing projects still interoperate.

I agree with these, though I didn't finish reviewing the block_witness_data.rs file, and I didn't have LSP support for the proto files.

Consumers/Interactors.:

1. the cli                                                                 (this repo)

2. https://github.com/zingolabs/zecwallet-mobile  (android and iOS)

3. https://github.com/zingolabs/zecwallet-lite        (the desktop)

4. https://github.com/zingolabs/lightwalletd           (the server)

In the interest of efficiency you should focus on a different part of the diff.

I also agree we need to consider these. Is there any chance there are more consumers outside our group of codebases you've listed here?

dannasessha commented 2 years ago

Just a soft suggestion: could we break this up into chunks, might that help?

zancas commented 2 years ago

You could open new PRs that contain well encapsulated chunks of this work, if you see how to.