Exported methods for iteration of verified allocations and claims

[anorth]

Implements exported method on the verified registry actor to iterate allocations and claims. The methods return IDs, which can then be looked up, though I could change it to return the full inflated objects too. These methods are not intended to be invoked on chain, but by off-chain parties like explorers and dashboards. Invoking methods like this can replace direct state inspection, which is a direction we should head over time.

(As a follow-up to this, we're planning to draft an FRC detailing what "deal" fields mean, and to make this backwards compatible with existing on-chain claims).

TODO

• Await FIP-0076 acceptance, including spec for this method
• Await a new FIP to specify this

Questions

• Do we need a mechanism to prevent invocation of methods like this on-chain. Do we need to stop user contracts from calling them, just because they could be very expensive? There is some discussion of this in the context of token standards at Native fungible token standard (FRC-0046) FIPs#407 (comment) @Kubuxu @raulk
• Should I rewrite MapMap::for_each_each as an external iterator, given that it's implemented in terms of them? It's probably a little more code, but more convenient to use from the outside. But there's no other callers here (yet). @Stebalien
• Is "invalid cursor" a good enough candidate for a common user exit code? If not, should I define an actor-specific exit code for this in verifreg, or stick with illegal argument? We have 5 common user exit codes unassigned. @Stebalien
• Do we need a FIP for this? It is an exported API. If so, I can roll it into DDO FIP before it goes to last call

[Stebalien]

Should I rewrite MapMap::for_each_each as an external iterator, given that it's implemented in terms of them? It's probably a little more code, but more convenient to use from the outside. But there's no other callers here (yet).

Yes. But I wouldn't bother implementing a manual iterator like I did. Instead, I'd use iterator combinators (returning an impl Iterator<Item=(&BytesKey, &BytesKey, &V)>.

You should be able to write this with something like outer.iter().flat_map(|(ok, inner)| inner.iter().map(|(ik, v)| (ok, ik, v))) (likely with some extra decoding, etc). You may need a move || { ... } to let the inner closure take ownership.

[Stebalien]

Is "invalid cursor" a good enough candidate for a common user exit code? If not, should I define an actor-specific exit code for this in verifreg, or stick with illegal argument? We have 5 common user exit codes unassigned. @Stebalien

Well...

1. I think we made a mistake and didn't reserve enough common exit codes. But changing that now could be... difficult (but may be worth trying).
2. IMO, "not found" may cover this? But I wouldn't object to having a new "invalid cursor" exit code.

[Stebalien]

Do we need a FIP for this? It is an exported API. If so, I can roll it into DDO FIP before it goes to last call

Yes.

[Stebalien]

Do we need a mechanism to prevent invocation of methods like this on-chain. Do we need to stop user contracts from calling them, just because they could be very expensive?

I don't really see why we'd have to prevent calling them unless we want to prevent them from being a public API. Being "expensive" isn't really a good reason.

However, if we do want to limit access, the standard way is to verify that the caller is the "system" actor.

[Stebalien]

Is this ready for a code review? Or are you just looking for answers to those questions?

[Kubuxu]

I don't really see why we'd have to prevent calling them unless we want to prevent them from being a public API. Being "expensive" isn't really a good reason.

The risk is that at some point they become impossible to call on-chain due to block/message gas limit.

[anorth]

You should be able to write this with something like outer.iter().flat_map(|(ok, inner)| inner.iter().map(|(ik, v)| (ok, ik, v))) (likely with some extra decoding, etc).

This has so far defeated me due to all handling of errors loading and iterating the inner map and type constraints on the closure. I need both ok and err branches to return an iter::Map<IterImpl<..>>.

I will probably attempt a manual iterator instead. No further review needed yet.

[anorth]

@Stebalien this is ready for review now.

Should I rewrite MapMap::for_each_each as an external iterator

After making some attempts, I've decided it's not worth the effort. I'm sure it's possible, but probably requires an explicit iterator object etc to handle plumbing the errors.

Is "invalid cursor" a good enough candidate for a common user exit code

I'm using ILLEGAL_ARGUMENT for the case where cursor is stale (the root CID of the structure has changed). Other issues like the keys not existing result in ILLEGAL_STATE, like every other HAMT lookup failure. Detecting this to assign a different exit code is possible, but again I deemed it not worthwhile.

Do we need a mechanism to prevent invocation of methods like this on-chain.

I have not implemented any such mechanism. I think this falls under allowing this capability to those who can use it safely rather than preventing it for everyone just because it's possible to screw up.

Do we need a FIP for this?

I will update FIP-0076 DDO to include this method, once the signature/code is approved. The branch is against master, rather than the DDO integration branch, but I won't merge this until the FIP is accepted.

[Stebalien]

A few nits but otherwise LGTM.

[Stebalien]

Ok, I tried to make this work but there's a fundamental problem: there's no way take ownership of the Hamt when iterating over it. So we have a problem:

1. Iterate over outer Hamt (outer_hamt).
2. Load inner Hamt (inner_hamt).
3. Iterate (iter_inner_hamt) over outer_hamt, borrowing inner_hamt in the iter_inner_hamt object.

Unfortunately, we'd need to store both inner_hamt and iter_inner_hamt in the same structure, which we can't do because iter_inner_hamt refers to inner_hamt (it's a self-referential object).

There are some ways to work around this, but they're kind of painful. Manually implementing the iterator doesn't get us anywhere.

[Stebalien]

TL;DR:

After making some attempts, I've decided it's not worth the effort. I'm sure it's possible, but probably requires an explicit iterator object etc to handle plumbing the errors.

It's possible to handle the errors, etc. even without an explicit iterator (although you do need to use itertools). But there's no reasonable way to deal with the lifetime issues any which way.

[Stebalien]

I'm using ILLEGAL_ARGUMENT for the case where cursor is stale (the root CID of the structure has changed). Other issues like the keys not existing result in ILLEGAL_STATE, like every other HAMT lookup failure. Detecting this to assign a different exit code is possible, but again I deemed it not worthwhile.

Yeah, that's fine.

[anorth]

Reverting this to draft and awaiting a new FIP to specify it.