search

Agoric / dapp-nft-drop

An example of a NFT Drop contract, with tests
3 stars 4 forks source link

`npm run test` throws error about non-frozen objects #1

Open tgrecojs opened 2 years ago

tgrecojs commented 2 years ago

Attempting to run the test code associated with this contract results in failing tests stemming from non-frozen objects. I've attempted to work through the error using harden but similar errors continue to appear, leading me to think that there might've been a change in the platform causing these tests to no longer run.

Quick screengrab below.

https://user-images.githubusercontent.com/6646552/146109728-95dd410d-fc7a-48da-863a-7ef3c12ce0eb.mp4

Any chance there's a quick fix for this? Migrating the code to use multiple duplicate harden functions seems like a lot of duplication so wondering if there's an easier fix for this 🤔

dckc commented 2 years ago

I think we did recently have a breaking API change that requires harden() in more places. I thought perhaps it was https://github.com/Agoric/agoric-sdk/pull/3892 but harden isn't mentioned there. @erights maybe you can help me find it?

meanwhile, @tgrecojs note this repo is a quick demo; don't expect too much by way of support. (I wonder if it belongs more in agoric-labs or some such.)

Also... any particular reason you're running npm run test rather than yarn test?

dckc commented 2 years ago

https://github.com/Agoric/agoric-sdk/pull/3892 is indeed the breaking change; we can tell from https://github.com/Agoric/agoric-sdk/blame/e0f7ee8884648973c63bbcae587ea36e51f58a64/packages/ERTP/src/amountMath.js#L74

I'm not sure why the changelog doesn't mention harden().

erights commented 2 years ago

Yes, that is the PR. It fixes ERTP to do proper input validation of its inputs. A consequence is that improper inputs which had previously been mistakenly accepted are now properly rejected. That PR does add the file https://github.com/Agoric/agoric-sdk/blob/master/packages/ERTP/docs/INPUT_VALIDATION.md#who-hardens to document the consequences of the stricter input validation, including the section "Who hardens?" that explains this issue in particular. I'm sorry we didn't draw more attention to this breaking change.

At the line in question:

https://github.com/Agoric/dapp-nft-drop/blob/a865ed758d808499f9366227b78511696e13d211/contract/test/test-NFTDropContractBasic.js#L61

the [1n] is invalid input that the previous code mistakenly allowed. Unfrozen, it is not a CopyArray, and so not a proper value for an Amount. Changing it to harden([1n]) should fix the problem. In general, hardening values before releasing them avoids communicating unintentional mutability, making code safer.

We recognize that the current obligation to use harden a lot is notationally unpleasant. On a speculative branch I'm experimenting with an approach that may let us safely omit harden much more often. But I don't know if this will work out. Until then, we should all continue to use harden to make our code more obviously safe against the consequences of unintended mutability.

tgrecojs commented 2 years ago

meanwhile, @tgrecojs note this repo is a quick demo; don't expect too much by way of support. (I wonder if it belongs more in agoric-labs or some such.)

The purpose is to provide an API documentation writing sample for the assessment of Dean + others on the team.

Also... any particular reason you're running npm run test rather than yarn test?

No particular reason - I guess it's a little bit of muscle-memory with using npm run. They both accomplish the same thing though. It's when you mix npm install and yarn install that you get into a sticky situation 🙂