h-REA / hREA

A ValueFlows / REA economic network coordination system implemented on Holochain and with supplied Javascript GraphQL libraries
https://docs.hrea.io
Other
142 stars 15 forks source link

omiting unitId (or passing the wrong one) in economic events for resources that have a specified unitId throws a very dense error #355

Closed Connoropolous closed 1 year ago

Connoropolous commented 1 year ago

This occurs when you try to call createEconomicEvent with a transfer action and you don't pass in the unitId

Uncaught (in promise) Error: Unexpected error value:
{ type: "error", data: { type: "ribosome_error", data: "Wasm runtime error while working with Ribosome: RuntimeError: 
WasmError { file: \"lib/hdk_records/src/lib.rs\", line: 111, error: Guest(\"Error in remote call 
WasmError { file: \\\"/Users/connor/.cargo/registry/src/github.com-1ecc6299db9ec823/holochain-0.0.152/src/core/ribosome/host_fn/call.rs\\\", line: 141, 
error: Host(\\\"Wasm runtime error while working with Ribosome: RuntimeError: 
WasmError { file: \\\\\\\"/Users/connor/.cargo/registry/src/github.com-1ecc6299db9ec823/holochain_wasmer_host-0.0.80/src/guest.rs\\\\\\\", line: 259, 
error: CallError(\\\\\\\"RuntimeError: unreachable\\\\\\\") }\\\") }\") }" } }
pospi commented 1 year ago

There are a few panic!s in that code. Suggest we log something upstream for Holochain Core to log those better.

Might have been introduced during some of the semi-recent event/resource logic updates, as most of those panics occur due to developer error with the event not being set up correctly.

Connoropolous commented 1 year ago

"Suggest we log something upstream for Holochain Core to log those better." that's a bit of it yeah, as I noticed the panics there, and here we can't see them.

Why panic vs making it a fallible operation?

pospi commented 1 year ago

'cos they shouldn't ever get triggered- probably unreachable! is a better descriptor for them. If it is those being triggered (presuming a lot to say so) then it's due to a bug in the code rather than a user error. That target_inventory_type attribute has to be set in earlier method calls in order for those methods to behave properly, it was the only way I could think of doing it at the time though acknowledge it's a little flaky :grimacing:

Connoropolous commented 1 year ago

I think we're thinking different instances.

I believe this is the one I'm referring to. This isn't so much developer error as based on something that the end user can pass

https://github.com/h-REA/hREA/blob/8a7b287a8827e6fb9681dc01e6f4cc0b08e80755/lib/vf_measurement/src/lib.rs#L39-L42

Connoropolous commented 1 year ago

I think the target_inventory_type one is fine

weswalla commented 1 year ago

@Connoropolous, Just to be sure, we want add and subtract https://github.com/h-REA/hREA/blob/e4d080c703b023353998fd537569a099a1c4a8aa/lib/vf_measurement/src/lib.rs#L43-L60 to return a RecordAPIResult instead?

This would mean updating the update_with method of the Updateable trait: https://github.com/h-REA/hREA/blob/e4d080c703b023353998fd537569a099a1c4a8aa/lib/hdk_records/src/record_interface.rs#L107-L115 since there is one instance of update_with that calls update_quantity, calling add or subtract: https://github.com/h-REA/hREA/blob/e4d080c703b023353998fd537569a099a1c4a8aa/zomes/rea_economic_resource/storage/src/lib.rs#L326

Connoropolous commented 1 year ago

oh I see update_with is not fallible... maybe we can find the place that will call update_with and circumvent it there instead

pospi commented 1 year ago

FWIW I do think we'll eventually want update_with to be fallible the same as for creates. Nothing in hREA yet needs it but I expect third party code might someday want to be able to handle invalid update payloads gracefully. If you want to make this the time and change the trait, feel free.

Connoropolous commented 1 year ago

That might be better because we need to make sure that all 3 unit types are ultimately compared. The units of the resource_inventoried_as if it exists, the units of the event, and the units of the to_resource _inventoried_as if it exists

Connoropolous commented 1 year ago

You were right all along Wesley

Connoropolous commented 1 year ago

Ok I'm leaving now 👋 I defer to your judgments!