fix: clone record before emitting event

[TimoGlastra]

Clone records before emitting them in events to avoid mutation later on.

Fixes #832

[codecov-commenter]

Codecov Report

Merging #833 (213ea0f) into main (28e0ffa) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #833      +/-   ##
==========================================
+ Coverage   87.71%   87.76%   +0.05%     
==========================================
  Files         439      439              
  Lines       10830    10854      +24     
  Branches     1837     1837              
==========================================
+ Hits         9499     9526      +27     
+ Misses       1269     1267       -2     
+ Partials       62       61       -1     

Impacted Files	Coverage Δ
...les/basic-messages/services/BasicMessageService.ts	92.59% <100.00%> (+1.28%)	⬆️
.../modules/connections/services/ConnectionService.ts	86.95% <100.00%> (+0.15%)	⬆️
...les/credentials/protocol/v1/V1CredentialService.ts	87.19% <100.00%> (-0.04%)	⬇️
...les/credentials/protocol/v2/V2CredentialService.ts	94.15% <100.00%> (-0.05%)	⬇️
.../modules/credentials/services/CredentialService.ts	100.00% <100.00%> (ø)
packages/core/src/modules/oob/OutOfBandModule.ts	87.16% <100.00%> (ø)
packages/core/src/modules/oob/OutOfBandService.ts	96.05% <100.00%> (+0.21%)	⬆️
...s/core/src/modules/proofs/services/ProofService.ts	85.79% <100.00%> (+0.12%)	⬆️
...ules/routing/services/MediationRecipientService.ts	83.52% <100.00%> (+0.38%)	⬆️
...re/src/modules/routing/services/MediatorService.ts	88.04% <100.00%> (+0.54%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28e0ffa...213ea0f. Read the comment docs.

[berendsliedrecht]

I like the addition of adding the emitStateChangedEvent and cloning/copying, but would it not make more sense to implement clone on the BaseRecord class? This way, we would avoid calling const clonedRecord = record.clone().

I also think that this will be forgotten in the future and cause problems again. I am sure of a solution but it might be nice to revisit this later on.

[berendsliedrecht]

Why not

Suggested change

	private emitStateChangedEvent(connectionRecord: ConnectionRecord, previousState: DidExchangeState | null) {
	private emitStateChangedEvent(connectionRecord: ConnectionRecord, previousState?: DidExchangeState) {

[TimoGlastra]

Events are emitted with previousState value null (which has a different meaning than undefined). If we change this now it would be a breaking change to events (because previousState will now be undefined)

[TimoGlastra]

This way, we would avoid calling const clonedRecord = record.clone().

How could we avoid calling record.clone? I like the idea of adding a clone to the base record, but instead of calling JsonTransformer.clone we'd now call record.clone() right? Or am I missing something?

[berendsliedrecht]

This way, we would avoid calling const clonedRecord = record.clone().

How could we avoid calling record.clone? I like the idea of adding a clone to the base record, but instead of calling JsonTransformer.clone we'd now call record.clone() right? Or am I missing something?

Yeah record.clone().

Maybe clone adds a property clone: true and where we want to passed a cloned object we require that property? This does not avoid calling it but we would not forget to call it at least.

[TimoGlastra]

I'm not 100% following you. Where would we add the clone property to? Could you give an example of how this would work?

I'm going to merge this now to unblock me in the AATH work, but happy to make a follow up PR

[berendsliedrecht]

I'm not 100% following you. Where would we add the clone property to? Could you give an example of how this would work?

I'm going to merge this now to unblock me in the AATH work, but happy to make a follow up PR

Yeah my apologies, not the best explanantion.

I want to enforce calling clone() on the record so we dont encounter more issues like this. My idea was that cloning would add a property to the record called shouldBeCloned and set it to true. Then every function that would modify the record would basically check if a property on the record called shouldBeCloned would be true.

This is for sure not the best solution and just an idea. Ignore this if its still too vague :).