Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: added method to duplicate predicate #3395

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

chore: added method to duplicate predicate #3395

wants to merge 10 commits into from
+60 −5

Conversation

YaTut1901
Copy link
Contributor

@YaTut1901 YaTut1901 commented Nov 14, 2024 •
edited
Loading

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 Vercel
Copy link

vercel bot commented Nov 14, 2024 •
edited
Loading

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
ts-docs-api ⬜️ Ignored (Inspect) Nov 18, 2024 6:56pm

@vercel Vercel
Copy link

vercel bot commented Nov 14, 2024

@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 Torres-ssf changed the title chore(): added method to duplicate predicate chore: added method to duplicate predicate Nov 15, 2024
@github-actions github-actions bot added the chore Issue is a chore label Nov 15, 2024
@Torres-ssf
Copy link
Contributor

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 HQ
Copy link

codspeed-hq bot commented Nov 17, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3395 will not alter performance

Comparing YaTut1901:YaTut1901/chore/predicate-duplication (ce466cf) with master (c904a98)

Summary

✅ 18 untouched benchmarks

danielbate
danielbate requested changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/account/test/predicate-functions.test.ts
});

expect(predicate.predicateData).toEqual(['DADA']);
expect(predicate.bytes[0]).toEqual(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Torres-ssf
Torres-ssf requested changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petertonysmith94 petertonysmith94 petertonysmith94 left review comments

@Torres-ssf Torres-ssf Torres-ssf requested changes

@danielbate danielbate danielbate requested changes

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

@arboleya arboleya Awaiting requested review from arboleya arboleya is a code owner

@Dhaiwat10 Dhaiwat10 Awaiting requested review from Dhaiwat10 Dhaiwat10 is a code owner

@nedsalk nedsalk Awaiting requested review from nedsalk nedsalk is a code owner

@maschad maschad Awaiting requested review from maschad maschad is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@YaTut1901 YaTut1901

Labels
chore Issue is a chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable creating a new predicate from an existing one
5 participants
@YaTut1901 @Torres-ssf @danielbate @petertonysmith94 @arboleya