In D19779#449787, @bytesofman wrote:

A couple of general issues

(1) it may not be the right approach to treat XEC and tokens both as generalized assets, with "atoms" meaning sats for XEC and atoms for tokens.

this works, but I can see it being difficult to extend to more complex txs, e.g. those that send both XEC and a token, or XEC and multiple tokens.

Even now, a typical tx is an Agora buy or sell, where parsing involves showing both correct token amounts and correct XEC amounts. So keeping sats and atoms distinct could prove more extensible.

That was my initial design, but it happens that having XEC being treated just like a token let me reuse a lot of code that would otherwise either be duplicated or "or typed" which makes it much harder to reason about. If it's about naming I can go with bits instead of atoms if you think it's less confusing.
Marlin is not trying to be fully featured, quite the opposite. In this context I would favor simplicity over extensibility, even if this require some refactors later on.

(2) More than enough features now to justify some integration tests...not necessarily with regtest, but something like Cypress, or perhaps there is a similar approach to Cashtab's react-testing-library "workflow" tests that could be implemented in this codebase. Would make it easy to review larger changes like this. As the app becomes more complex, reviews are forced to accept more on faith. From the dev perspective, the app is also getting to the point where adding these kinds of tests from nothing to full app coverage becomes daunting enough to dissuade entirely. So tests would become both more important and less likely to be initialized.

This one I agree 100%. Because there few features I can't have a ton of unit tests but I definitely need to add some functional tests. That's the next item on my list for Marlin.

That was my initial design, but it happens that having XEC being treated just like a token let me reuse a lot of code that would otherwise either be duplicated or "or typed" which makes it much harder to reason about. If it's about naming I can go with bits instead of atoms if you think it's less confusing.
Marlin is not trying to be fully featured, quite the opposite. In this context I would favor simplicity over extensibility, even if this require some refactors later on.

the decision is good provided it lines up with the intended direction of the app

Tail of the build log:

     sitional balance -- shows spend transitional then available balance after mempoo               
     l and finalization WS (failed).png                                                             
  -  /work/apps/marlin-wallet/web/cypress/screenshots/transitional-balance.cy.ts/Tran     (1280x720)
     sitional balance -- shows spend transitional amount in fiat then finalizes to up               
     dated fiat primary (settings before tx) (failed).png                                           


================================================================================

  (Run Finished)


       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  history.cy.ts                            00:12        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  main.cy.ts                               00:07        5        5        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  mnemonic.cy.ts                           00:17        7        7        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✖  send.cy.ts                               189ms        1        -        1        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✖  transitional-balance.cy.ts               01:14        8        6        2        -        - │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✖  2 of 5 failed (40%)                      01:51       27       24        3        -        -  

--------------------------------------------------------------------------------

  Debug faster with full visibility.

  Record to Cypress Cloud and get instant access to full test details and replays.
  Inspect the DOM, network events, and console logs exactly as they ran in CI.

  >> https://on.cypress.io/cloud-get-started

--------------------------------------------------------------------------------
 ELIFECYCLE  Command failed with exit code 3.
<i> [webpack-dev-server] Gracefully shutting down. To force exit, press ^C again. Please wait...
Error: Command failed with exit code 3: pnpm run cypress:run
    at makeError (/work/node_modules/.pnpm/execa@5.1.1/node_modules/execa/lib/error.js:60:11)
    at handlePromise (/work/node_modules/.pnpm/execa@5.1.1/node_modules/execa/index.js:118:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:103:5) {
  shortMessage: 'Command failed with exit code 3: pnpm run cypress:run',
  command: 'pnpm run cypress:run',
  escapedCommand: '"pnpm run cypress:run"',
  exitCode: 3,
  signal: undefined,
  signalDescription: undefined,
  stdout: undefined,
  stderr: undefined,
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}
/work/apps/marlin-wallet/web:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  marlin-wallet-web@1.8.2 test:e2e: `start-server-and-test 'pnpm run dev' http://localhost:3000 'pnpm run cypress:run'`
Exit status 1
 ELIFECYCLE  Command failed.
Build marlin-wallet-e2e-tests failed with exit code 1

In D19779#451062, @bytesofman wrote:

• it's still difficult for me to think about XEC and tokens in the same bucket, with atoms sometimes meaning sats and sometimes meaning atoms. but it does work and it makes sense with the architecture of the app. potentially an opportunity for ecash-wallet to better support some of this stuff. Seems like marlin is doing heavier lifting here than it "should" need to do

I couldn't find a better name than atoms, which is used through the codebase for tokens but really isn't a specific L2 keyword (could be atomicBase or something more verbose like that, but in the end the meaning is the same). We could have some toolbox somewhere in ecash-wallet. I looked at how it's done in Firma and Cashtab: the first one uses a 3rd party lib and the latter string operations. So the first is much cleaner but adds an additional security maintenance, and the latter has to deal with complicated code and bad performance (even if this is not really impactful in a wallet). Not sure what would be the best option for ecash-wallet.

• At this point Marlin is no longer a simple app, at least in terms of codebase. I would normally be super wary of doing it all with vanilla JS. The main reason is that testing is much harder than with say react and react-testing-library. However with cypress, this is no longer an issue at all, and the tests are actually much better (and representative of cross-device behavior) than react-testing-library. The tests in Cashtab are comprehensive but also incredibly complex. Cashtab tests have been exceptionally difficult to maintain through dep upgrades, typescript upgrades. The required mocks often require so much babysitting that it's like the app or screen is rebuilt for a test. Improved testing tools and AI will probably lead to more cases like this, emphasis on no-framework code and in-environment testing; both "better" but previously either impossible or not worth the learning curve.

Cypress is an incredible tool, thanks for the suggestion! It's amazing how the tests are especially easy to read. I also like the idea of running against a chronik node like we do for other integration tests. This would require the apps to support regtest (but that should be easy enough) and to find a way to communicate between the test setup and the frontend automation, which might be more difficult. I need to dig into cypress' internals to see if that's possible. This would make it possible to remove the chronik mocks used for the tests entirely (they would be constructed dynamically by a script instead).

• it's not necessarily intuitive that the user needs to click on the eCash logo in order to toggle between firma and ecash. I don't know if adding anything would really help though. Perhaps a dropdown-like carat to the left? But maybe it's better without. Once you know what to do, it's quite simple.

OK I will explore the options.

Some unrelated improvements I noticed in testing

• Mnemonic entry inputs could have autocapitalization disabled

It's disabled already, let's discuss offline to debug this.

• it would be nice if, when editing the "Amount" input, the user could still see the "Hold to send" button. right now you must manually dismiss the keyboard and then send

Good idea, I'll explore the options here.

• I'm able to run the cypress tests locally but only if I run cypress open from web/; I get a "not installed" error if i try pnpm run cypress:open ... I dunno this might be a local issue for me, not sure if it's a code issue at all

It's expected to run in web/, I did not add a script to run from the app root. I can do that tho

• A good goal (mb) for both Cashtab and Marlin would be regtest integration cypress tests. In this way, we could avoid some of the mocking complexity, actually test websocket interactions, chained txs, wallet handling of external incoming txs, maybe even how multiple wallets interact with each other.

Agreed it would be nice, see above