chore: added static method to duplicate predicate #3432
Conversation
YaTut1901
requested review from
digorithm,
arboleya,
Torres-ssf,
Dhaiwat10,
danielbate,
nedsalk,
petertonysmith94 and
maschad
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@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 @danielbate @petertonysmith94 hello, could you please check this implementation and if that fits point me which docs it should be documented in? |
CodSpeed Performance ReportMerging #3432 will degrade performances by 54.51%Comparing Summary
Benchmarks breakdown
|
| static fromInstance< | ||
| TData extends InputValue[] = InputValue[], | ||
| TConfigurables extends { [name: string]: unknown } | undefined = { [name: string]: unknown }, | ||
| >(instance: Predicate<TData, TConfigurables>) { | ||
| return new (class { | ||
| data: TData; | ||
| configurableConstants: TConfigurables; | ||
|
|
||
| constructor() { | ||
| this.data = instance.predicateData; | ||
| this.configurableConstants = {} as TConfigurables; | ||
| } | ||
|
|
||
| withData(data: TData): this { | ||
| this.data = data; | ||
| return this; | ||
| } | ||
|
|
||
| withConfigurableConstants(configurableConstants: TConfigurables): this { | ||
| this.configurableConstants = configurableConstants; | ||
| return this; | ||
| } | ||
|
|
||
| build(): Predicate<TData, TConfigurables> { | ||
| return new Predicate({ | ||
| bytecode: instance.initialBytecode, | ||
| abi: instance.interface.jsonAbi, | ||
| provider: instance.provider, | ||
| data: this.data, | ||
| configurableConstants: this.configurableConstants, | ||
| }); | ||
| } | ||
| })(); | ||
| } |
There was a problem hiding this comment.
| static fromInstance< | |
| TData extends InputValue[] = InputValue[], | |
| TConfigurables extends { [name: string]: unknown } | undefined = { [name: string]: unknown }, | |
| >(instance: Predicate<TData, TConfigurables>) { | |
| return new (class { | |
| data: TData; | |
| configurableConstants: TConfigurables; | |
| constructor() { | |
| this.data = instance.predicateData; | |
| this.configurableConstants = {} as TConfigurables; | |
| } | |
| withData(data: TData): this { | |
| this.data = data; | |
| return this; | |
| } | |
| withConfigurableConstants(configurableConstants: TConfigurables): this { | |
| this.configurableConstants = configurableConstants; | |
| return this; | |
| } | |
| build(): Predicate<TData, TConfigurables> { | |
| return new Predicate({ | |
| bytecode: instance.initialBytecode, | |
| abi: instance.interface.jsonAbi, | |
| provider: instance.provider, | |
| data: this.data, | |
| configurableConstants: this.configurableConstants, | |
| }); | |
| } | |
| })(); | |
| } | |
| static fromInstance< | |
| TData extends InputValue[] = InputValue[], | |
| TConfigurables extends { [name: string]: unknown } | undefined = { [name: string]: unknown }, | |
| >(instance: Predicate<TData, TConfigurables>, overrides?: PredicateParams = {}) { | |
| return new Predicate({ | |
| bytecode: instance.initialBytecode, | |
| abi: instance.interface.abi, | |
| provider: instance.provider, | |
| data: instance.data, | |
| configurableConstants: instance.configurableConstants, | |
| ...overrides, | |
| }); | |
| } |
Would the following not be simpler? We'd need to keep a reference to the initial configurableConstants with this example, but this could be omitted.
There was a problem hiding this comment.
I was thinking about something like this, but decide not to store constants. So you think that way is better?
There was a problem hiding this comment.
I'd be in favour of this route, but the team may have differing opinions.
There was a problem hiding this comment.
@arboleya @Torres-ssf @danielbate will there be any other comments?
There was a problem hiding this comment.
Maintaining different APIs around predicate instantiation doesn't seem right to me, should be fluid from one to the next, the builder feels unnecessary IMO. I'm in favour of this change.
Additionally, does this function need to be static? Then we could avoid having to pass predicate as first param? 🤔
There was a problem hiding this comment.
But in case if we have way to override any of fields via PredicateParams then for me there is no reason to have this method. Predicate can be created with constructor
There was a problem hiding this comment.
The reason why I decided to make a builder pattern here is to encapsulate logic of overriding only constants and data while remain other fields unchangeable
There was a problem hiding this comment.
what do you think? @danielbate @petertonysmith94
There was a problem hiding this comment.
You could pick out the fields that are required to override:
overrides?: Pick<PredicateParams, 'data' | 'configurableConstants'> = {}There was a problem hiding this comment.
okay, that fits better. I will implement like this
| static fromInstance< | ||
| TData extends InputValue[] = InputValue[], | ||
| TConfigurables extends { [name: string]: unknown } | undefined = { [name: string]: unknown }, | ||
| >(instance: Predicate<TData, TConfigurables>) { | ||
| return new (class { | ||
| data: TData; | ||
| configurableConstants: TConfigurables; | ||
|
|
||
| constructor() { | ||
| this.data = instance.predicateData; | ||
| this.configurableConstants = {} as TConfigurables; | ||
| } | ||
|
|
||
| withData(data: TData): this { | ||
| this.data = data; | ||
| return this; | ||
| } | ||
|
|
||
| withConfigurableConstants(configurableConstants: TConfigurables): this { | ||
| this.configurableConstants = configurableConstants; | ||
| return this; | ||
| } | ||
|
|
||
| build(): Predicate<TData, TConfigurables> { | ||
| return new Predicate({ | ||
| bytecode: instance.initialBytecode, | ||
| abi: instance.interface.jsonAbi, | ||
| provider: instance.provider, | ||
| data: this.data, | ||
| configurableConstants: this.configurableConstants, | ||
| }); | ||
| } | ||
| })(); | ||
| } |
There was a problem hiding this comment.
Maintaining different APIs around predicate instantiation doesn't seem right to me, should be fluid from one to the next, the builder feels unnecessary IMO. I'm in favour of this change.
Additionally, does this function need to be static? Then we could avoid having to pass predicate as first param? 🤔
There was a problem hiding this comment.
@danielbate in my previous PR you proposed to make it static. For me it seems good either
Summary
Added static method fromInstance which returns builder anonymous class which can be populated with constants and data and be assembled to Predicate instance.
Checklist