Add GM for base asset ID

[Dentosal]

Spec PR: FuelLabs/fuel-specs#561

[MitchTurner]

So, I'd say that the SUT of this test is op::gm_args and the condition is GMArgs::BaseAssetId. It's confusing since we can't call gm_args natively and instead we call transact, so you could also say that transact is the SUT in that sense.

Either way, I think the structure of the test should reflect what the SUT is. In this case I include it in a gm_args_script method, so it's a little indirect but cleans up the test in a good way IMO:

// given
let base_asset_id = AssetId::from([5; 32]);
let params = ConsensusParameters {
        base_asset_id,
        ..Default::default()
    };

// when
let arg = GMArgs::BaseAssetId;
let size = AssetId::LEN;
let script = gm_args_script(arg, size);

let logged_data = run_script_and_get_logged_data(params, script);

// then
let expected = params.base_asset_id.to_bytes();
let actual = logged_data;
assert_eq!(expected, actual);

So the name of the test could be something like gm_args__base_asset_id_retrieves_value_in_params

[Dentosal]

I think doing that would make the test case less readable. Note how this is already an integration test, not a unit test, and it still has minimal indirection which is quite nice. I experimented a bit with something like run_script_and_get_logged_data and going that way seems to lead essentially duplicating TransactionBuilder if we want to make it reusable.

[xgreenx]

Before we were returning an ExpectedInternalContext error in the case of the internal context. Do we need to do the same?

[Dentosal]

Good catch. The new behavior is what the specs say. I see no reason to disallow internal contexts here.

[xgreenx]

The specification says that it is allowed only in the context of the predicate:

And in our codebase predicate context is external context:

So it looks like we need ti check that it is external context=)

[Dentosal]

But the code already performs the check: The context.predicate() returns None outside predicate contexts, leading to TransactionValidity panic. Previously calling gm/GetVerifyingPredicate panicked with ExpectedInternalContext which was definitely incorrect. Now if we check with something like

if context.is_external() { // pseudocode
  result = context.predicate().map(...).ok_or(PanicReason::TransactionValidity)?;
} else {
  return PanicReason::ExpectedExternalContext;
}

then the only additional check we're actually performing is that Call contexts return ExpectedExternalContext instead of TransactionValidity. Is the issue just returning a wrong panic reason? Am I missing something here?

[xgreenx]

Ohh, I missed the moment that predicate already is doing this check inside=D Hmm, maybe using of another error will make more sense, because TransactionValidity is a strange choice

[Dentosal]

I'm going to merge this as-is, since TransactionValidity is (only) used for this kind of things currently. We can do a rename in a separate PR if needed.

[Voxelot]

looks like there are merge conflicts now

[xgreenx]

The specification says that it is allowed only in the context of the predicate:

And in our codebase predicate context is external context:

So it looks like we need ti check that it is external context=)

[xgreenx]

Ohh, I missed the moment that predicate already is doing this check inside=D Hmm, maybe using of another error will make more sense, because TransactionValidity is a strange choice