[adapter][move] Relax object/pure arg ordering. remove return values

[tnowacki]

• Relaxed ordering on entry points, so object and pure arguments can be intermixed
• Added new enum CallArg to support this
• Removed return values from entry points
• Removed CallResult

A lot of the input object logic was gutted as the adapter rarely needs the owned object, so it now defers to loading an object reference.
I removed the loading of immediate dependencies for publish transactions. It will fail fairly quickly inside the VM, but I can put the eager logic back (I'm just not sure it is needed)

[tnowacki]

There are some gas tests that are failing, but I'm having a bit of trouble debugging them, maybe @lxfind, you will be able to help?
One example

---- authority::gas_tests::test_publish_gas stdout ----
thread 'authority::gas_tests::test_publish_gas' panicked at 'assertion failed: `(left == right)`
  left: `GasCostSummary { computation_cost: 112, storage_cost: 93, storage_rebate: 0 }`,
 right: `GasCostSummary { computation_cost: 359, storage_cost: 93, storage_rebate: 0 }`', sui_core/src/unit_tests/gas_tests.rs:238:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[lxfind]

We need to load the immediate dependencies for safety reasons.
Imagine two validators are receiving the same publish transaction, but one validator doesn't have one of the dependent package on-chain. Without reading the immediate dependencies during handle_transaction phase, both will pass and lock the objects. But then when it comes to execution, one would succeed with publish, while the other will fail (but the failed one will still charge gas, i.e. mutate the gas object). Now the two validators have inconsistent state.

[tnowacki]

We need to load the immediate dependencies for safety reasons.

Gotcha, makes sense. And do we not need to do it recursively? Because we assume the non-immediate deps are in a sane state?
(EDIT: And I suppose a similar argument for not checking the deps of the call?)

[lxfind]

We need to load the immediate dependencies for safety reasons.

Gotcha, makes sense. And do we not need to do it recursively? Because we assume the non-immediate deps are in a sane state? (EDIT: And I suppose a similar argument for not checking the deps of the call?)

Correct, if a validator is aware of a package, it must have successfully executed the cert that publishes that package, which means all the dependencies of that package must be on-chain. So we don't need to check non-immediate dependencies.

[lxfind]

Thanks! Amazing work. This is going to take a few iterations to review.

A few high-level comments:

1. Why do we change to not pass in the objects to adapter? Is it really cleaner that way? If so, perhaps add a TODO somewhere that when we construct the TemporaryStore, we should just consume the input objects to avoid unnecessary clones.
2. This PR seems to be doing many things at once, making it difficult to review.. I recommend in the future we try to break up PRs.

[lxfind]

Is this the same as &object.clone()?

[tnowacki]

No, the object here is already owned (object: Object), so the clone wouldn't do anything. This is a trick for getting around the lifetimes. If you try doing Some(object) => &object, it complains that the reference will not live long enough. This is a trick that forces the lifetime outside of the match, thus letting it live long enough.

[tnowacki]

Why do we change to not pass in the objects to adapter? Is it really cleaner that way? If so, perhaps add a TODO somewhere that when we construct the TemporaryStore, we should just consume the input objects to avoid unnecessary clones.

I tried cleaning up the logic that that had a count of the number of the objects, and then used take off of the front of the iterator. Thats when I realized that all of those objects were already inside of the TemporaryStore, so no TODO needed, its already the case that this is happening.

This PR seems to be doing many things at once, making it difficult to review.. I recommend in the future we try to break up PRs.

I apologize about that, and I still can try to do that now that it all makes a bit more sense to me. A bit of that comes from my unfamiliarity with the codebase. This PR is really the result of me just changing things and having the type checker yell, and I then fix it until it goes away.

[tnowacki]

I tried to revert most of the spurious changes

[lxfind]

Adding @patrickkuo @666lcz for visibility on the changes to the gateway API.

[lxfind]

I tried cleaning up the logic that that had a count of the number of the objects, and then used take off of the front of the iterator. Thats when I realized that all of those objects were already inside of the TemporaryStore, so no TODO needed, its already the case that this is happening.

I think we still have two copies of the objects: one in objects_by_kind in authority.rs, and one in temporary store. Eventually we could consider have the construction of AuthorityTemporaryStore to consume the objects, which is what I think needs a TODO here.

[tnowacki]

I think we still have two copies of the objects: one in objects_by_kind in authority.rs, and one in temporary store. Eventually we could consider have the construction of AuthorityTemporaryStore to consume the objects, which is what I think needs a TODO here.

Ah, yeah there is no real work to remove the clone there, just missed it (like no rearchitecting, just removing the & and reordering a few calls)

[lxfind]

LGTM. Thank you!
Please take a look at the comments.
Also, @patrickkuo please confirm that the changes in the rest server and rpc server are good.

[patrickkuo]

rest and rpc_gateway changes LGTM