Add owner policy #872

AurelienFT · 2024-11-18T15:11:21Z

FuelLabs/fuel-specs#617

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

CHANGELOG.md

fuel-asm/src/args.rs

fuel-tx/src/tests/valid_cases/transaction.rs

fuel-tx/src/transaction/validity.rs

acerone85 · 2024-11-22T10:59:21Z

fuel-vm/src/tests/metadata.rs

@@ -622,6 +624,12 @@ fn get_transaction_fields() {
        op::eq(0x10, 0x10, 0x11),
        op::and(0x20, 0x20, 0x10),

+        op::movi(0x19, 0x00),
+        op::movi(0x11, owner_idx as Immediate18),


question for me to understand: why Immediate18? By looking at the specs and the implementation of GTF, I would expect Immediate12 here?

I don't know I just followed the existing pattern. Maybe @Dentosal knows

acerone85 · 2024-11-22T11:05:22Z

fuel-vm/src/tests/metadata.rs

@@ -622,6 +624,12 @@ fn get_transaction_fields() {
        op::eq(0x10, 0x10, 0x11),
        op::and(0x20, 0x20, 0x10),

+        op::movi(0x19, 0x00),
+        op::movi(0x11, owner_idx as Immediate18),
+        op::gtf_args(0x10, 0x19, GTFArgs::PolicyOwner),


Another question for me to understand here.
I guess the PolicyOwner field is taken from the transaction included in the frame pointer. What is the value of the register 0x19 here used for? From the specs i can infer that it is related to the part of the field that we want to read, but it is not clear if this is the start or end offset or something else.

Anyway, not related to this PR (also probably this value is ignored since I'd expect the owner value to be fixed size).

Same as above I jsut copy pasted the existing. cc @Dentosal

acerone85 · 2024-11-22T11:08:07Z

Another question for me to understand:
What is the motivation behind this change? I.e. why is it important for transactions to have an explicit user, and how it is going to be used by applications?

Co-authored-by: Andrea Cerone <22031682+acerone85@users.noreply.github.com>

…into add_owner_policy

AurelienFT · 2024-11-22T14:20:46Z

Another question for me to understand: What is the motivation behind this change? I.e. why is it important for transactions to have an explicit user, and how it is going to be used by applications?

The motivation is explained here : FuelLabs/fuel-specs#617 but in other word the smart contract sometimes wants to be able to know who is the caller (partially because dev are use to EVM msg.sender and also to have a common standard of who is the owner is needed by contract). This will allow contracts to have the language when speaking about owners instead of the current anarchy.

rafal-ch · 2024-11-26T15:37:48Z

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

@@ -133,6 +133,8 @@ pub enum ValidityError {
    TransactionInputsMax,
    TransactionOutputsMax,
    TransactionWitnessesMax,
+    TransactionOwnerIndexDoesntExist,


a nit:

Suggested change

TransactionOwnerIndexDoesntExist,

TransactionOwnerIndexOutOfBounds

I'm ok with that. Changed.

rafal-ch · 2024-11-26T15:46:21Z

fuel-tx/src/transaction/validity.rs

+        if owner >= tx.inputs().len() {
+            Err(ValidityError::TransactionOwnerIndexDoesntExist)?
+        }
+        // SAFETY: `owner` is guaranteed to be a valid index because it was checked above.
+        if !&tx.inputs()[owner].input_owner().is_some() {
+            Err(ValidityError::TransactionOwnerInputHasNoOwner)?
+        }


can we avoid manual safety check with something like:

if tx .inputs() .get(owner) .and_then(|input| input.input_owner()) .is_none() { return Err(ValidityError::TransactionOwnerInputHasNoOwner); }

?

Ok I will change that. (not the whole code to mentionned because otherwise we will loose the two different errors)

rafal-ch · 2024-11-26T15:47:05Z

fuel-tx/src/transaction/validity.rs

@@ -321,6 +322,24 @@ where
    Ok(())
 }

+pub(crate) fn check_owner<T>(tx: &T) -> Result<(), ValidityError>


Do you think adding some UTs to this function makes sense?

I don't really think so it's well tested with the other tests and it's not a practice we currently have for validation rules. I don't find this one more sensible or difficult to understand.

xgreenx · 2024-11-26T23:22:51Z

I marked this PR as a Draft for now until we resolved https://forum.fuel.network/t/proposal-for-note-inputs/7094 proposal=)

The base branch was changed.

AurelienFT added 6 commits November 16, 2024 23:21

Add expiration policy, some tests are still failing.

d1748bc

Fix tests

ac9d47b

Update changelog

d57fcd0

Update test and default expiration vlaue

ab81aaf

Add owner policy

3be6418

Format and changelog

3a9a42d

AurelienFT self-assigned this Nov 18, 2024

add tests owner policy

4274f0a

AurelienFT marked this pull request as ready for review November 19, 2024 13:58

AurelienFT requested review from xgreenx, Dentosal, MitchTurner and Voxelot as code owners November 19, 2024 13:58

AurelienFT requested a review from a team November 19, 2024 13:58

AurelienFT mentioned this pull request Nov 19, 2024

Add owner policy FuelLabs/fuel-specs#618

Draft

5 tasks

AurelienFT added 2 commits November 20, 2024 11:26

Add check input owner exists

e90ec27

Update owner to optional field.

31da0ab

acerone85 reviewed Nov 21, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

acerone85 reviewed Nov 21, 2024

View reviewed changes

fuel-asm/src/args.rs Show resolved Hide resolved

petertonysmith94 mentioned this pull request Nov 22, 2024

Support policy owner FuelLabs/fuels-ts#3415

Closed

acerone85 reviewed Nov 22, 2024

View reviewed changes

fuel-tx/src/tests/valid_cases/transaction.rs Outdated Show resolved Hide resolved

acerone85 reviewed Nov 22, 2024

View reviewed changes

fuel-tx/src/transaction/validity.rs Outdated Show resolved Hide resolved

acerone85 reviewed Nov 22, 2024

View reviewed changes

AurelienFT and others added 3 commits November 22, 2024 14:30

Update CHANGELOG.md

325e6e1

Co-authored-by: Andrea Cerone <22031682+acerone85@users.noreply.github.com>

Update test and split check in function

4e9bb3d

Merge branch 'add_owner_policy' of https://github.com/FuelLabs/fuel-vm …

3f90b00

…into add_owner_policy

acerone85 previously approved these changes Nov 24, 2024

View reviewed changes

rafal-ch reviewed Nov 26, 2024

View reviewed changes

Change error name and make a check on input safe.

ab4e2f9

AurelienFT requested a review from rafal-ch November 26, 2024 17:46

xgreenx marked this pull request as draft November 26, 2024 23:22

Base automatically changed from add_expiration_policy to master December 2, 2024 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add owner policy #872

Add owner policy #872

AurelienFT commented Nov 18, 2024 •

edited

Loading

acerone85 Nov 22, 2024

AurelienFT Nov 22, 2024

acerone85 Nov 22, 2024

AurelienFT Nov 22, 2024

acerone85 commented Nov 22, 2024

AurelienFT commented Nov 22, 2024

rafal-ch Nov 26, 2024

AurelienFT Nov 26, 2024

rafal-ch Nov 26, 2024

AurelienFT Nov 26, 2024

rafal-ch Nov 26, 2024

AurelienFT Nov 26, 2024

xgreenx commented Nov 26, 2024

	TransactionOwnerIndexDoesntExist,
	TransactionOwnerIndexOutOfBounds

Add owner policy #872

Are you sure you want to change the base?

Add owner policy #872

Conversation

AurelienFT commented Nov 18, 2024 • edited Loading

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 commented Nov 22, 2024

AurelienFT commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx commented Nov 26, 2024

AurelienFT commented Nov 18, 2024 •

edited

Loading