Node/Solana: Shim watcher cleanup

[bruce-riley]

No description provided.

[johnsaigle]

I think some of the error handling is a little too general and may cause some bugs down the line. Otherwise, I made some suggestions to improve error messages and fix typos in test names.

[johnsaigle]

Right now I believe this check is too general: this returns an error if the inner instruction's program index matches the Wormhole Program's, regardless of the contents. I think instead what should be flagged here is the case where another "verified core event" occurs.

So the more targeted check would be something like:

// Ensure that only one verified core event appears in each set of inner instructions
if foundIt && verifiedCoreEvent {
 // return error 
 }

In practice this might be fine, but maybe there's a valid case (now or down the line) where the Wormhole program needs to be interacted with twice in the same inner instruction set.

[banescusebi]

Yeah, it looks pretty restrictive to me in the sense that any sequence of instructions where there's an instruction that returns true on shimVerifyCoreMessage(inst.Data), and then contains any subsequent instruction where the following condition holds inst.ProgramIDIndex == whProgramIndex, will result in an error.

[johnsaigle]

I think it's OK to add this the instruction to the alreadyProcessed set even if foundIt is false.

[johnsaigle]

Same as the case above -- this will catch and throw an error for any interaction with the shim program, even if this isn't a second shim event. So, this code might end up looking like this:

		} else if inst.ProgramIDIndex == shimProgramIndex {
			if !verifiedCoreEvent {
				return fmt.Errorf("detected an inner shim message event instruction before the core event for shim instruction %d, %d: %w", outerIdx, idx, err)
			}
			thisEvent, err := shimParseMessageEvent(s.shimMessageEventDiscriminator, inst.Data)
			if err != nil {
				return fmt.Errorf("failed to parse inner shim message event instruction for shim instruction %d, %d: %w", outerIdx, idx, err)
			}
			if messageEvent != nil && thisEvent != nil && len(thisEvent) >  0 {
				return fmt.Errorf("detected multiple shim message event instructions when there should not be at instruction %d, %d", outerIdx, idx)
			}
			alreadyProcessed.add(outerIdx, idx)
		}

[johnsaigle]

Shouldn't the loop continue in case there are multiple verifiedCoreEvent occurrences? If not, it would be good to reword this to something like "In the integrator (inner instruction) case, additional core event and shim event pairs are allowed."

Basically my concern with this function is to make sure that:

• there's a one-to-one correspondance of core events and shim events in all cases (i.e. they're always matched)
• there are no "extra events" -- e.g. in the Direct/top-level case, there should only be one core event and one shim event

[johnsaigle]

Suggested change

	func TestShimDirectWithExtraWhEventBeforeShimEventShouldFails(t *testing.T) {
	func TestShimDirectWithExtraWhEventBeforeShimEventShouldFail(t *testing.T) {

[johnsaigle]

Suggested change

	func TestShimTopLevelShouldFailIfNoInstructionsShouldFail(t *testing.T) {
	func TestShimTopLevelEmptyInstructionsShouldFail(t *testing.T) {

Just a suggestion to remove the duplicate use of ShouldFail

[johnsaigle]

Suggested change

	func TestShimInnerShouldFailIfNoInstructionsShouldFail(t *testing.T) {
	func TestShimProcessInnerInstructions_OutOfBoundsStartIndexShouldFail(t *testing.T) {

This name is super long but the point is just to clarify that what's being tested here is the "start index" value relative to the length of the inner instructions.

[johnsaigle]

Suggested change

	return false, fmt.Errorf("startIdx %d is greater than or equal to %d", startIdx, len(innerInstructions))
	return false, fmt.Errorf("startIdx %d is out of bounds of slice innerInstructions (length: %d)", startIdx, len(innerInstructions))

From the error log it's not clear why e.g. "startIdx 3 is greater than or equal to 3" is a problem

[banescusebi]

Suggested change

	return false, fmt.Errorf("topLevelIndex %d is greater than %d", topLevelIdx, len(tx.Message.Instructions))
	return false, fmt.Errorf("topLevelIndex %d is greater than the total number of instructions in the tx message %d", topLevelIdx, len(tx.Message.Instructions))

[banescusebi]

Yeah, it looks pretty restrictive to me in the sense that any sequence of instructions where there's an instruction that returns true on shimVerifyCoreMessage(inst.Data), and then contains any subsequent instruction where the following condition holds inst.ProgramIDIndex == whProgramIndex, will result in an error.