-
Notifications
You must be signed in to change notification settings - Fork 43
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
Query Filter Comparator Remaping #1016
base: develop
Are you sure you want to change the base?
Conversation
7e672c2
to
74b7a72
Compare
318aaa4
to
8757e07
Compare
8757e07
to
cf7a682
Compare
switch v := visitor.(type) { | ||
case *pgDSLParser: | ||
v.VisitEventSigFilter(f) | ||
v.VisitEventSubkeyFilter(f) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't combine the eventSigFilter
and eventSubkeyFIlter
. Unlike in evm where the event sig is just "topic 0" the event sig in Solana has nothing to do with the event subkeys. They are two separate columns in the db table, one has to compare the event_sig
column and the other references an index into the subkey_values
column.
Also the event sigs have a specific format, it must always be an 8 byte discriminator--we should validate that if we can I guess. Whereas the subkey values are fields in a struct, and can be any primitive type; these have to be converted with NewIndexedValue
before storing in the appropriate slot of subkey_values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh. I messed that one up. I don't know what I was thinking. So essentially we need both: the event sig filter when subkey filters are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at evm again, event sig is always provided. I'll make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just backing out these changes to parser.go should be fine, as long as you're okay with merging my changes first (which add VisitEventSubkeyFilter
while keeping VisitEventSigFilter
)
|
||
for idx, comp := range comparator.ValueComparators { | ||
// need to do a transform and then extract the value for the subkey | ||
newValue, err := b.modifier.TransformToOnChain(comp.Value, itemType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't call this function, unless we modify the codec to skip the Borsh-encoding step.
I'm still not clear on whether we actually need to call this at all, but if we do then it either has to be without the Borsh-encoding or we'll need to decode it as soon as it finishes encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransformToOnchain
is a modifier function and doesn't handle encoding/decoding.
modifier commoncodec.Modifier | ||
key solana.PublicKey | ||
remapper remapHelper | ||
indexedSubkeys map[string]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would lean towards calling this subKeys
rather than indexedSubkeys
. The latter might give someone the impression that we support non-indexed subKeys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the config where the name is IndexedSubkeys
where I think it is good to imply they are indexed.
@@ -174,6 +174,40 @@ func NewIDLInstructionsCodec(idl IDL, builder commonencodings.Builder) (commonty | |||
return typeCodecs, nil | |||
} | |||
|
|||
func NewIDLEventCodec(idl IDL, builder commonencodings.Builder) (commontypes.RemoteCodec, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two definitions of this and eventFieldsAsStandardFields
now, probably happened because I included that in my PR and then you rebased onto it. I think they are identical, so just get rid of one or the other shouldn't matter.
func (b *eventReadBinding) decodeLog(ctx context.Context, log *logpoller.Log, into any) error { | ||
itemType := codec.WrapItemType(false, b.namespace, b.genericName, codec.ChainConfigTypeEventDef) | ||
|
||
// decode non indexed topics and apply output modifiers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an evm-specific comment. For Solana, it should just say "decode event data".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout.
cf7a682
to
f510fdd
Compare
Quality Gate failedFailed conditions |
Description
This PR adds support for remapping chain agnostic filters to solana specific filters.
NONEVM-1063
Requires Dependencies