Skip to content

Implement debugging support for the Advanced Transaction Builder #230

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

Open
rkalis opened this issue Oct 29, 2024 · 16 comments
Open

Implement debugging support for the Advanced Transaction Builder #230

rkalis opened this issue Oct 29, 2024 · 16 comments
Labels
javascript-sdk Relates to the CashScript JavaScript SDK
Milestone

Comments

@rkalis
Copy link
Member

rkalis commented Oct 29, 2024

No description provided.

@rkalis rkalis added this to the v0.11 milestone Oct 29, 2024
@mr-zwets mr-zwets added the javascript-sdk Relates to the CashScript JavaScript SDK label Oct 29, 2024
@mr-zwets
Copy link
Member

In issue #172 we documented a note of the initial difficulties of implementing this:
However we did not add notes from our meeting with Jason, but our assumption about the need to generate multiple scenarios was indeed affirmed.

Note about multiple input handling / advanced transaction builder:

Earlier we mentioned that we're not sure how multiple inputs can be tested at once, since there can only be one 'slot' field in a >scenario. We'll have to discuss this with Jason, but our expectation is that we'd need to generate multiple scenarios, one for each >input, and check that they all pass.

However, we noticed that our scenarios do support P2PKH inputs, which are evaluated separately. So it does seem like it's >possible to evaluate multiple inputs using multiple scripts. We will need to look into this further.

@rkalis
Copy link
Member Author

rkalis commented Jan 14, 2025

Today we started looking at @kiok46's new PR. We extracted his example file into an automated test with documented output. We looked through the output template, which all looks to be correct. We did notice that some tests from the "regular" libauth template fixtures were failing (likely due to naming convention changes), and there were TypeScript issues surrounding the breaking changes.

We also want to further investigate if we can minimise the breaking changes to the TransactionBuilder API and we will need to merge the new TransactionBuilder file with the existing one, and the new LibauthTemplate file with the existing one as well.

Next session we want to expand the test cases a bit by adding more contracts and more configurations of inputs/outputs (e.g. multiple inputs spending with the same function). Then we want to start refactoring / merging the new and existing files as mentioned above.

When we are doing refactors, it will be good to also see if we can address these issues while doing so: #204

We should also update the tests under e2e/transaction-builder to use Mocknet with the new debugging functionality.

@rkalis
Copy link
Member Author

rkalis commented Jan 27, 2025

Today we added two extra test cases to the multi-contract template generation. We found 1 bug with scenario naming in there that we added a TODO for and will get back to later. We then started refactoring / backporting the advanced transaction builder changes. We updated the InputOptions and Unlocker types as discussed with @kiok46 on Telegram to abstract away some complexity from the user and avoid breaking changes to the API.

Next session we will start by updating the tests under e2e/transaction-builder to use Mocknet and the new debug functionality. We should also add a e2e test for the bug case we found as mentioned above. Then we want to update all the tests in debugging.test.ts so that we know all the debug functionality is properly tested. We should also add a few more multi-contract test cases to that file so that we're also properly testing specifically the multi-contract functionality.

After updating the tests and fixing any potential critical bugs/issues, we'll cut a new next release before doing deeper refactors.

@rkalis
Copy link
Member Author

rkalis commented Feb 4, 2025

Today we made some improvements to the TransactionBuilder tests and split the e2e tests from the "API tests", but we still need to continue with most of the debugging testing that we mentioned in the previous comment. Additionally, we want to update all of the e2e tests to use the advanced transaction builder and add some multi-contract e2e tests to the newly created e2e/MultiContract.test.ts.

@mr-zwets
Copy link
Member

mr-zwets commented Feb 10, 2025

I spent some extra time on this in preparation.
I started updating the e2e tests and this is the current result:

✅ successfully converted to TransactionBuilder & pushed:
announcement, tokenCategoryCheck, mecenas & hodlvault

🟧 converted with minor issue & pushed with issue oustanding:
p2pkh: should fail when not enough satoshis are provided in utxos (TransactionBuilder does not check this)
edit transferWithTimeout: worked on second implementation, first got an error "recipient_sig missing from template"

❌ failing to convert, not-pushed:
bigint (failed to generate template)

should fail require statement when providing a number that fits within 64 bits

    expect(received).rejects.toThrow(expected)

    Expected constructor: FailedRequireError
    Received constructor: TypeError

    Received message: "bytes.reduce is not a function"

          381 |     .filter(([, arg]) => !(arg instanceof SignatureTemplate))
          382 |     .map(([input, arg]) => {
        > 383 |       const encodedArgumentHex = binToHex(arg as Uint8Array);
              |                                  ^
          384 |       const prefixedEncodedArgument = encodedArgumentHex.length > 0 ? `0x${encodedArgumentHex}` : '';
          385 |       return [snakeCase(input.name), prefixedEncodedArgument] as const;
          386 |     });

      at binToHex (../../node_modules/@bitauth/libauth/src/lib/format/hex.ts:66:9)
      at src/LibauthTemplate.ts:383:34
                at Array.map (<anonymous>)
      at generateTemplateScenarioParametersValues (src/LibauthTemplate.ts:382:6)
      at generateTemplateScenarios (src/advanced/LibauthTemplate.ts:271:14)
      at getLibauthTemplates (src/advanced/LibauthTemplate.ts:428:9)
      at TransactionBuilder.debug (src/TransactionBuilder.ts:161:35)
      at TransactionBuilder.send (src/TransactionBuilder.ts:182:16)
      at Object.<anonymous> (test/e2e/BigInt.test.ts:37:10)
      at Object.toThrow (../../node_modules/expect/build/index.js:218:22)
      at Object.<anonymous> (test/e2e/BigInt.test.ts:40:39)

🤔 not tried:
p2pkh-tokens (tests a bunch of safety/sanety checks not present in the TransactionBuilder class)

@rkalis
Copy link
Member Author

rkalis commented Feb 11, 2025

Today we started updating the old fixtures to work with the new LibauthTemplate functionality. While doing so we got lost trying to get our snake case function to work correctly. In the end we just used a library for it, but we'll need to replace that some time down the line (but definitely not before the coming @next release).

We'll likely have another session in the coming days to finish updating all the tests. If no major issues are found after we've updated the tests, then we'll try to merge this into next and get a @next release out.

@mr-zwets
Copy link
Member

mr-zwets commented Feb 11, 2025

I ported the e2e test for transferWithTimeout & bigint to use the TransactionBuilder also! 😄

we should check if the e2e tests work on chipnet still after the changes.

remaining is debugging.test.ts, e2e/P2PKH-tokens.test.ts, and the outstanding TODO in /fixture/libauth-template/multi-contract-fixtures.ts

potentially we could also look to add a tests to MultiContract.test.ts before any next release

@rkalis
Copy link
Member Author

rkalis commented Feb 14, 2025

Today we made some changes to ensure that the updated e2e tests still work on chipnet. Then we updated the debugging tests to use TransactionBuilder.

Next session we will have to finish porting the e2e tests with e2e/P2PKH-tokens.test.ts (or skip it if it has too much custom checks). We also should add some tests to MultiContract.test.ts and add some multi-contract tests to debugging.test.ts.

After that we should be ready to deploy a @next release and merge @kiok46's PR into next and continue refactors from there.

@rkalis
Copy link
Member Author

rkalis commented Feb 18, 2025

Today we started adding to the MultiContract.test.ts file. We ran into 3 bugs on the way:

The first bug concerned scenarios for Contract A not having access to data from Contract B (which is necessary in order to do proper introspection). We were able to solve this bug.

The second bug was a small one, where P2PKH inputs were not being indexed correctly, we also solved this.

The third bug is that, while we are generating multiple scenarios, TransactionBuilder.debug() only evaluates the first scenario. We'll need to update it so that it evaluates all scenarios.

Lastly, we also noticed there is a potential for naming conflicts (both for constructor parameters with the same name between contracts, and also when using the same name for contracts). We'll have to further document these in test cases so that we can figure out how to deal with those.

The naming conflict can wait until after a @next release, but the outstanding bug should be resolved before.

So next session, we plan to resolve that bug and finish the rest of the tests + update the multicontract fixtures. After that we can aim to deploy a @next release.

@rkalis
Copy link
Member Author

rkalis commented Feb 25, 2025

Today we managed to fix the remaining issue where not all scenarios were evaluated. We also updated the fixtures and fixed some issues that we accidentally introduced last time. So now none of the tests are failing and we have reasonable enough test coverage for multi-contract debugging to cut a @next release.

We fixed one part of the naming collisions that we considered blocking, but we're leaving the rest of those naming collision issues for after the next release. We didn't manage to add any multi-contract tests to debugging.test.ts, but that is not blocking, so we can also do that after the next release.

We also noticed that the "snake-casing" of names/parameters etc everywhere might not be necessary, and adds unneeded complexity and potential for bugs. So we want to look into removing the snake-casing.

We didn't get to cut the release today, but we're planning to do another session this week to get the release and docs out.

@mr-zwets
Copy link
Member

mr-zwets commented Mar 5, 2025

If we look up all .to( occurrences we can see there's 4 main places we still need to move to the TransactionBuilder class

  • packages/cashscript/test/fixture/libauth-template/fixtures.ts
  • all /examples need to be updated
  • packages/cashscript/test/network/FullStackNetworkProvider.test.ts
  • packages/cashscript/test/e2e/P2PH-tokens.test.ts

the following test filess only test the .functions not the .unlock

  • Contract.test.ts
  • Contract.types.test

@mr-zwets
Copy link
Member

mr-zwets commented Mar 18, 2025

I came across two issues with the debugging functionality and added failing tests for them:

  • added failing test case TwtContract timeout + twtContract transfer using 2 signature variables in multi-contract-fixtures.ts
  • added failing test case in Multi-Contract-Debugging tests which does not fail on final verify

@rkalis
Copy link
Member Author

rkalis commented Mar 25, 2025

Today we addressed the first of the two issues that @mr-zwets found by adding a reference to the corresponding input index for every entity and unlock script. To do so, we created new entities / unlock scripts for every input index, even if they use the same contract or contract function. We also reverted a previous change that we made to have multiple scenarios per unlock script.

We still need to update most of the fixtures to reflect these changes. And next time we also need to update the naming of scenarios to also include the input index (rather than the current incrementing number).

@rkalis
Copy link
Member Author

rkalis commented Mar 28, 2025

Today we updated the fixtures and fixed a small bug that we ran into where P2PKH inputs were not fully working with the new TransactionBuilder.

After, we got started on @mr-zwets' second issue. We figured out that this occurred because there's an {} in the input of a transaction of a scenario. This {} indicates that the unlocking bytecode for that input is the same as 'slot'. However, with multiple different contracts, this is incorrect, since there will be several different unlocking bytecodes.

We added what we think it should look like to one of the fixtures, and next session we'll try to resolve this bug.

@rkalis
Copy link
Member Author

rkalis commented Apr 1, 2025

Today we fixed the second bug that @mr-zwets found and updated 1 test accordingly. @mr-zwets will update the rest of the fixtures later today and after that we will cut a new prerelease.

@mr-zwets
Copy link
Member

mr-zwets commented Apr 2, 2025

I noticed 2 more issues with the debugging

  • The final verify doesn't throw the correct error message for the correct contract (added failing test case)
  • In most places we use contractName which comes from the artifact but in others we use contract.name
    these two are the same by default this.name = artifact.contractName; but in the CashScript playground developers can give the contracts unique names (as there can be multiple instances of the same contract

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript-sdk Relates to the CashScript JavaScript SDK
Projects
None yet
Development

No branches or pull requests

2 participants