Closed arvidn closed 3 years ago
I have to admit, I'm very confused of the relationship between CLVMObject
and SExp
. On the one hand SExp
is deriving from CLVMObject
, but there are a few places where we check type(x)
is CLVMObject
. If those are changed to isinstance()
, things break.
I would have expected that CLVMObject
would be a Concept (in C++ terminology) or a Trait (in rust terminology).. or just a protocol anything could implement to provide the view into a clvm tree of values.
I don't actually think this patch makes sense on its own. All the places that check isinstance(x, CLVMObject)
and type(x) == CLVMObject
are suspicious to me. Don't we just want to check whether x
implements our concept/trait/protocol?
Yes, I've also experimented with the idea of changing isinstance(obj, CLVMObject)
to hasattr(obj, "pair") and hasattr(obj, "atom")
and it seems to work reasonably well. So then we can replace CLVMObject
with any "duck type" that has .pair
and .atom
. This means we can drop in a rust-based replacement quite easily.
I was experimenting with that today, and I was not successful. it's a mystery to me.
in a way I think we want SExp
to be a wrapper around something that provides "atom" and "pair", to provide the higher level operations.
I did this experiment in a triple-branch of clvm
, clvm_tools
and clvm_rs
with other changes and improvements, so at this time I don't have a minimal set of changes necessary to make this work.
in a way I think we want
SExp
to be a wrapper around something that provides "atom" and "pair", to provide the higher level operations.
Yes, exactly.
allow SExp to be constructed from anything implementing the CLVMObject protocol
namely a
pair
andatom
member. The comment inCLVMObject
seem to suggest this was the intention