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.

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

Fuel Mint support Part 1 - Implement HyperFuel query selection for Mint receipts #214

Merged
merged 11 commits into from
Sep 19, 2024

Conversation

DZakh
Copy link
Member

@DZakh DZakh commented Sep 18, 2024

No description provided.

@@ -136,12 +136,33 @@ let registerContractHandlers = (
{{#with chain_config.network_config.hyperfuel_config as | hyperfuel_config |}}
module(HyperFuelWorker.Make({
let chain = chain
let contracts = contracts
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally to get rid of the old contract config

name: "{{event.name.capitalized}}",
logId: Types.{{contract.name.capitalized}}.{{event.name.capitalized}}.sighash,
{{!-- mint: {{#if event.mint}}true{{else}}false{{/if}}, --}}
eventMod: module(Types.{{contract.name.capitalized}}.{{event.name.capitalized}}),
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally to pass everything via the event options and get rid of the generated eventMod (use it only for types)

events: array<event>,
}

let makeGetRecieptsSelectionOrThrow = (~contracts: array<contract>) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted it to a factory, I'm going to write a few tests tomorrow. Ideally to do the same for HyperSync. Also, what's good about it that it'll allow to get rid of Evm specific fields from EventMod

})

let contractsReceiptQuery = shouldApplyWildcards
? contractsReceiptQuery->Array.concat(wildcardLogSelection)
Copy link
Member Author

Choose a reason for hiding this comment

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

Bye bye concat

Comment on lines -101 to -108
let receipts = contractsReceiptQuery->Js.Array2.map((
q
): HyperFuelClient.QueryTypes.receiptSelection => {
rootContractId: ?q.addresses,
receiptType: receiptTypeSelection,
rb: q.rb,
txStatus: txStatusSelection,
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Like how everything became much lighter after moving it to hyperFuelWorker

@DZakh DZakh changed the title Fuel Mint support Part 1 Fuel Mint support Part 1 - Implement HyperFuel query selection for Mint receipts Sep 18, 2024
Comment on lines 80 to 85
| addresses => {
if mint.contents == #NonWildcard {
selection
->Js.Array2.push({
rootContractId: addresses,
receiptType: mintReceiptTypeSelection,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any chance you can leave some comments on the conditional logic here? It's a little difficult to understand from just the code.

let txStatusSelection = [1]

let mint: ref<[#None | #Wildcard | #NonWildcard]> = ref(#None)
let nonWildcardRbsByContract = Js.Dict.empty()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is rbs short for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I understand it's plural of "rb" 👍🏼

Comment on lines 36 to 48
let maybeWildcardLogSelection = switch wildcardRbs {
| [] => None
| wildcardRbs =>
Some(
(
{
receiptType: logDataReceiptTypeSelection,
txStatus: txStatusSelection,
rb: wildcardRbs,
}: HyperFuelClient.QueryTypes.receiptSelection
),
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this always the None case? I don't see where wildcardRbs gets any values added before this.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, caught this as well, while I writing tests

let module(Event) = event.eventMod
let {isWildcard} = Event.handlerRegister->Types.HandlerTypes.Register.getEventOptions
switch event {
| {mint: true} => mint := (isWildcard ? #Wildcard : #NonWildcard)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like "mint" can be over written with "#Wildcard or #NonWildcard", what should happen if both appear?

Comment on lines +119 to +124
}

selection
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whew 😅 this function was quite difficult to read. Maybe we can break it up a bit. I worry about logic errors creeping in when it's difficult to read end to end.

Copy link
Collaborator

@JonoPrest JonoPrest left a comment

Choose a reason for hiding this comment

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

The only blocking thing where I think there is a bug is where maybeWildcardLogSelection gets defined in the worker function. The rest of my comments are just about readability. Let me know if I'm wrong about the bug

@DZakh
Copy link
Member Author

DZakh commented Sep 19, 2024

@JonoPrest I finished writing tests and found 2 bugs and then one more. I hope the code became a little bit more readable. At least it should have 100% coverage now.

Comment on lines 148 to 152
addresses: [
{{#each contract.addresses as | address |}}
"{{address}}"->Address.unsafeFromString,
{{/each}}
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these static addresses never used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. They are taken from contractConfig outside of the HyperFuelWorker

| {logId} => {
let rb = logId->BigInt.fromStringUnsafe
if isWildcard {
if event.isWildcard === Some(true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should go in the pattern match like the above cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to duplicate the let rb = logId->BigInt.fromStringUnsafe line

Comment on lines +242 to +253

module Set = {
type t<'value>

/*
* Constructor
*/
@ocaml.doc("Creates a new `Set` object.") @new
external make: unit => t<'value> = "Set"

@ocaml.doc("Creates a new `Set` object.") @new
external fromEntries: array<'value> => t<'value> = "Set"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of interest why rebind this whole module? Just to be explicit about what the bindings are?

Copy link
Member Author

Choose a reason for hiding this comment

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

To prevent the case when you expect it to use the bindings, but then it unexpectedly used Belt version instead

Copy link
Collaborator

@JonoPrest JonoPrest left a comment

Choose a reason for hiding this comment

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

Ok great, think it's a lot more understandable what the various cases are now 👍🏼

Nice one Dmitry 👊🏼

@DZakh DZakh merged commit 8246af4 into main Sep 19, 2024
1 check passed
@DZakh DZakh deleted the dz/fuel-mint-event branch September 19, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants