chore: added method to duplicate predicate

[YaTut1901]

• Closes Enable creating a new predicate from an existing one #3392

Summary

Added method toNewInstance which returns new instance of predicate with same abi, loaderBytecode, provider and different configurableConstants and data to be passed:

toNewInstance(params: {
  configurableConstants?: TConfigurables;
  data?: TData;
}): Predicate<TData, TConfigurables> { }

Checklist

• All changes are covered by tests (or not applicable)
• All changes are documented (or not applicable)
• I reviewed the entire PR myself (preferably, on GH UI)
• I described all Breaking Changes (or there's none)

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
ts-docs-api	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 29, 2024 9:00am

[vercel]

@YaTut1901 is attempting to deploy a commit to the Fuel Labs Team on Vercel.

A member of the Team first needs to authorize it.

[Torres-ssf]

Hi @YaTut1901,

Great work on this! We really appreciate your help.

Could you edit the PR description, specifically this part:

Closes https://github.com/FuelLabs/fuels-ts/issues/3392

and change it to:

- Closes https://github.com/FuelLabs/fuels-ts/issues/3392

Thank you!

[codspeed-hq]

CodSpeed Performance Report

Merging #3395 will not alter performance

_{Comparing YaTut1901:YaTut1901/chore/predicate-duplication (c21ba6e) with master (ef94263)}

Summary

✅ 18 untouched benchmarks

[danielbate]

Would be good to have an e2e test inside fuel-gauge that uses a predicate via toNewInstance

[danielbate]

This check seems redundant, can we check the entire bytecode?

[Torres-ssf]

@YaTut1901 Thanks for your help 🙏

However, I am skeptical that the current solution will fully resolve the issue.

When a new predicate is instantiated, its bytes property does not always reflect the original predicate bytes with default configurable values.

Configurable values are embedded directly into the predicate bytes during processing. Consequently, the stored bytes reflect the instance's configurable values rather than the pristine, default state. This can be observed on processPredicateData and setConfigurableConstants.

When using toNewInstance to create a new predicate, the bytes property inherits the values from the previous instance, which might include non-default configurable values. This makes it impossible to rely on toNewInstance to produce a new instance with the pristine, default configurable values.

It’s possible that someone might want to use toNewInstance without setting new configurable values, relying instead on the defaults.

I believe we need to store the predicate's pristine bytes separately, those that remain untouched by configurable values.

[YaTut1901]

@YaTut1901 Thanks for your help 🙏

However, I am skeptical that the current solution will fully resolve the issue.

When a new predicate is instantiated, its bytes property does not always reflect the original predicate bytes with default configurable values.

Configurable values are embedded directly into the predicate bytes during processing. Consequently, the stored bytes reflect the instance's configurable values rather than the pristine, default state. This can be observed on processPredicateData and setConfigurableConstants.

When using toNewInstance to create a new predicate, the bytes property inherits the values from the previous instance, which might include non-default configurable values. This makes it impossible to rely on toNewInstance to produce a new instance with the pristine, default configurable values.

It’s possible that someone might want to use toNewInstance without setting new configurable values, relying instead on the defaults.

I believe we need to store the predicate's pristine bytes separately, those that remain untouched by configurable values.

Hm, okay, I will check this point more precisely

[Dhaiwat10]

Maybe this can be a static method on the Predicate class like so:

const predicateInstance = new Predicate(...);
const newInstance = Predicate.fromInstance(predicateInstance);

Feels like a better API to me. Thoughts? @FuelLabs/sdk-ts

[YaTut1901]

@Torres-ssf I pushed a commit where refactored predicate class to keep configurable constants in separate field and set them to bytecode at the moment of operation execution. I see this as the option to keep initial bytecode untouched. And in this case there is no need to have a method to create new predicate object - existing one can be modified. What do you think about this approach?

[YaTut1901]

Hey, @Torres-ssf @danielbate @petertonysmith94 @Dhaiwat10, could anybody comment on my refactoring?

[Torres-ssf]

Hey, @Torres-ssf @danielbate @petertonysmith94 @Dhaiwat10, could anybody comment on my refactoring?

@YaTut1901 We’ll need to add some e2e tests to validate the changes introduced here.

These tests should be added to packages/fuel-gauge/src/predicate. A new test suite can be created for this purpose.

The test suite should ensure the following:

1 - Configurable Values: When creating a new Predicate instance from an existing one, ensure that any newly provided configurable values are correctly applied to the new instance.

2 - Default Values: If no configurable values are provided, confirm that the new instance uses the hardcoded default values from the Predicate code, not those from the previous instance.

Additionally, remember that configurable values are embedded within the predicate bytes. Any changes to these values change the predicate address, resulting in a new instance with a different address. Consequently, the latest instance will be a new account, without any of the previous funds added.

We also need to add some documentation for this. We need to tell our users about this new feature and how they can use it.

[YaTut1901]

Hey, @Torres-ssf @danielbate @petertonysmith94 @Dhaiwat10, could anybody comment on my refactoring?

@YaTut1901 We’ll need to add some e2e tests to validate the changes introduced here.

These tests should be added to packages/fuel-gauge/src/predicate. A new test suite can be created for this purpose.

The test suite should ensure the following:

1 - Configurable Values: When creating a new Predicate instance from an existing one, ensure that any newly provided configurable values are correctly applied to the new instance.

2 - Default Values: If no configurable values are provided, confirm that the new instance uses the hardcoded default values from the Predicate code, not those from the previous instance.

Additionally, remember that configurable values are embedded within the predicate bytes. Any changes to these values change the predicate address, resulting in a new instance with a different address. Consequently, the latest instance will be a new account, without any of the previous funds added.

We also need to add some documentation for this. We need to tell our users about this new feature and how they can use it.

I can do this, but overall does this change worth it? Or there can be other easier way?

[Torres-ssf]

I can do this, but overall does this change worth it? Or there can be other easier way?

@YaTut1901 Implementing a method to re-instantiate a predicate from an already instantiated one?

Yes, I believe this change is worthwhile as it enhances the flexibility and clarity of how Predicate instances handle configurable values. That said, I understand this implementation, along with the required tests and documentation, is not trivial and requires a deeper understanding of the stack.

If you feel this might be too much to take on right now, that's completely fine. You're more than welcome to step away from it, and we can reassign or revisit it later. Just let us know how you'd like to proceed.

[YaTut1901]

I can do this, but overall does this change worth it? Or there can be other easier way?

@YaTut1901 Implementing a method to re-instantiate a predicate from an already instantiated one?

Yes, I believe this change is worthwhile as it enhances the flexibility and clarity of how Predicate instances handle configurable values. That said, I understand this implementation, along with the required tests and documentation, is not trivial and requires a deeper understanding of the stack.

If you feel this might be too much to take on right now, that's completely fine. You're more than welcome to step away from it, and we can reassign or revisit it later. Just let us know how you'd like to proceed.

I meant the way I proposed, like not having method to re-instantiate predicate, but have an option to update values on existing one. It is possible in case when all changes to initial bytecode are applied only when transaction is sent

[danielbate]

I meant the way I proposed, like not having method to re-instantiate predicate, but have an option to update values on existing one. It is possible in case when all changes to initial bytecode are applied only when transaction is sent

The reason though is that if the values are changed on a predicate and the underlying bytecode has changed, then you have a new predicate address which inherently is a new predicate instance, as @Torres-ssf has mentioned.

Therefore it makes sense for us to replicate this behaviour in the SDK so it is clear to the user that when these values change, they are dealing with a new predicate instance. Hence it is important to document and test this behaviour due to the risk of loss of funds, as we are dealing with a new account. With that in mind, we can not do it at TX submission.

[YaTut1901]

I meant the way I proposed, like not having method to re-instantiate predicate, but have an option to update values on existing one. It is possible in case when all changes to initial bytecode are applied only when transaction is sent

The reason though is that if the values are changed on a predicate and the underlying bytecode has changed, then you have a new predicate address which inherently is a new predicate instance, as @Torres-ssf has mentioned.

Therefore it makes sense for us to replicate this behaviour in the SDK so it is clear to the user that when these values change, they are dealing with a new predicate instance. Hence it is important to document and test this behaviour due to the risk of loss of funds, as we are dealing with a new account. With that in mind, we can not do it at TX submission.

Oh, okay, got the point. So I will take it

[YaTut1901]

created new PR with implementation from scratch: #3432