feat: Configurable Base Asset ID

[bvrooman]

Related issues:

• References Use parameter to define base asset ID #164

[Dentosal]

Surprisingly clean changeset, I expected this was hardcoded in more places. I'm not sure wheter we need more test coverage to protect against possible issues with this, mostly because I don't know what ares would beed testing. Generally this looks good to me, and the only potential issue I found is this:

Does this part of the code use AssetId::default() for a good reason?

fuel-vm/fuel-tx/src/transaction/validity.rs

Lines 365 to 370 in 487e19f

	Output::Change { asset_id, .. }
	if asset_id != &AssetId::default()
	&& input_asset_id == asset_id =>
	{
	Some(())
	}

[bvrooman]

Does this part of the code use AssetId::default() for a good reason?

Going forward, AssetId::default() should only be used in tests. Now that it's part of the consensus parameters, a default Asset ID isn't really meaningful, and is not tied to the configured value. Rather, it's just a convenience function that produces "some" Asset ID that can be used in tests.

I will create a tech-debt ticket to place the impl Default under a cfg(test) compile guard, or replace default with a test-specific name, like test_default(). Issue here: #577

[Dentosal]

LGTM; would appreciate another review as well as I might have missed something

[Voxelot]

@bvrooman looks like there's some new clippy lints after merging master

[Dentosal]

I still would appreciate another reviewer, but I think this is good to go.

[MitchTurner]

I'm curious why the asset id ended up on the "fee" configs. Seems like a misleading name at least. I'll read through the code a bit, but I assume that it's used in similar places as the fee stuff.

[bvrooman]

I'm curious why the asset id ended up on the "fee" configs. Seems like a misleading name at least. I'll read through the code a bit, but I assume that it's used in similar places as the fee stuff.

As I understand it, fees are generally paid in the base asset. So we can assert that the asset we use for fees is the base asset throughout the VM. In a lot of areas, we hardcoded this assumption (using AssetId::BASE). But it's conceivable that fees can be paid in other assets, like a native token, and we can configure that.

[MitchTurner]

As I understand it, fees are generally paid in the base asset. So we can assert that the asset we use for fees is the base asset throughout the VM. In a lot of areas, we hardcoded this assumption (using AssetId::BASE). But it's conceivable that fees can be paid in other assets, like a native token, and we can configure that.

Yeah. I guess that makes sense in the context of places that are dealing with fees.
I feel like the base asset is relevant to more places than just fees though. So I'd be comfortable if it was just its own field on ConsensusParameters. Thoughts?

[bvrooman]

As I understand it, fees are generally paid in the base asset. So we can assert that the asset we use for fees is the base asset throughout the VM. In a lot of areas, we hardcoded this assumption (using AssetId::BASE). But it's conceivable that fees can be paid in other assets, like a native token, and we can configure that.

Yeah. I guess that makes sense in the context of places that are dealing with fees. I feel like the base asset is relevant to more places than just fees though. So I'd be comfortable if it was just its own field on ConsensusParameters. Thoughts?

I can understand motivation behind moving it to the top level in ConsensusParameters. I think that is worth exploring. If anyone else feels strongly about this, please feel free to weigh in. In the meantime, I will try it out locally to see how that looks.

[bvrooman]

I pushed a commit that moves base_asset_id to the root of ConsensusParameters. This was fairly straightforward. It did introduce extra plumbing, since we now have to pass base_asset_id along with other fee parameters in some areas. But overall, the changes are minor.

[Voxelot]

lgtm - i also participated in pairing on this and already familiar with the changes.