Enable ChainReader to read PDA account state #1003

amit-momin · 2025-01-07T22:07:49Z

Enable the ChainReader to read PDA account state

Added a PDADefintion field to the read definition to allow us to configure the fields needed for a PDA account read
PDADefintion includes a Prefix and a Seeds field
- Prefix is a static string that is converted to bytes and prepended to the list of seeds used for PDA calculation
- Seeds is a list of seeds that are expected to be provided through params
Updated accountReadBinding's GetAddress method to intake params to use for seeds when calculating the PDA
Added a new method to accountReadBinding to encode and build the seeds list

silaslenihan

This all looks great to me! Really readable and straightforward. Not sure if you're done adding functionality for CCIP but this should unblock the interface tests

silaslenihan · 2025-01-08T14:31:00Z

pkg/solana/chainreader/chain_reader.go

-	))
+	var reader readBinding
+	// Create PDA read binding if Seeds config is non-nil
+	// Note: Empty seeds list is a valid configuration for a PDA so a length check is intentionally skipped


Would an empty seeds list just be a normal account read?

I think FindProgramAddress still outputs a different programID even with empty seeds since it's still applying the bump seed. I think every PDA in the on-chain code uses some sort of seed though. So guess it wouldn't hurt to also validate empty seeds for our current use case but I wanted to keep this open in case something changes in the future.

pkg/solana/chainreader/pda_read_binding.go

ilija42 · 2025-01-15T22:10:19Z

pkg/solana/codec/solana.go

+		case SeedPubKey:
+			typedCodec = NewPublicKey()
+		case SeedUint64:


why can't we reuse asStruct here so that every type is handled?

I was thinking asStruct seemed strongly tied to IDL types and since PDA seeds aren't officially defined in the IDL I thought it made sense to keep this separate. But your comments making me rethink that. I think I can use IDLField in place of SeedDefinition and reuse the existing IDLType instead of my custom enum. I'll give this a shot

ilija42 · 2025-01-15T22:12:18Z

pkg/solana/codec/solana.go

+		var typedCodec commonencodings.TypeCodec
+		switch field.Type {
+		case SeedPubKey:
+			typedCodec = NewPublicKey()


Why is special handling for public key needed? Why doesn't regular public key encoded by Solana codec work?

Sorry this was just a miss on my part. I see this now which would get automatically picked up once I address the previous comment.

ilija42 · 2025-01-15T22:15:15Z

pkg/solana/chainreader/chain_reader.go

+		}
+		reader = newPdaReadBinding(namespace, genericName, readDefinition.PDAPrefix)
+	} else {
+		if err := s.addCodecDef(true, namespace, genericName, codec.ChainConfigTypeAccountDef, idl, codec.NilIdlTypeDefTy, readDefinition.InputModifications); err != nil {


What does this do? There are no inputs for non PDA account reads

I think this is unused for normal account reads. For encoding PDAs, we only really need the new addSeedsCodecDef method I added. But when I tried to remove this, I started getting some errors. I can debug and see what's going on but since this was already in the existing code I opted to keep it as is

my bad actually, I wrote this code lol

I thought I left it as a placeholder for PDA to be input, not sure why it throws errors

ilija42 · 2025-01-15T22:22:28Z

pkg/solana/chainreader/chain_reader.go

+func (s *SolanaChainReaderService) addSeedsCodecDef(namespace, genericName string, seedDefs []codec.SeedDefinition, modCfg commoncodec.ModifiersConfig) error {
+	mod, err := modCfg.ToModifier(codec.DecoderHooks...)
+	if err != nil {
+		return err
+	}
+	// Append seed suffix to differentiate the entry for encoding seeds from the account read entry
+	seedEntry, err := codec.NewSeedEntry(genericName, mod, seedDefs)
+	if err != nil {
+		return fmt.Errorf("failed to create a codec entry for seed definitions %v: %w", seedDefs, err)
+	}
+
+	// Seed codec entry is only used for encoding. Read type is not used by WrapItemType for encoding string.
+	s.parsed.EncoderDefs[codec.WrapItemType(true, namespace, genericName, "")] = seedEntry
+
+	return nil
+}
+


I think this can go away in favour of just using addCodecDef(). You just need to define seeds as a new IDL type and handle it like events, args and accounts are handled here CreateCodecEntry() which is a func used by addCodecDef().

ilija42

looks good

EasterTheBunny

LGTM

cl-sonarqube-production · 2025-01-17T20:10:46Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

Implemented PDA account reading in ChainReader

228bf30

amit-momin temporarily deployed to integration January 7, 2025 22:08 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 7, 2025 22:12 — with GitHub Actions Inactive

Fixed linting

723c222

amit-momin temporarily deployed to integration January 7, 2025 22:35 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 7, 2025 22:40 — with GitHub Actions Inactive

silaslenihan reviewed Jan 8, 2025

View reviewed changes

amit-momin added 3 commits January 14, 2025 17:39

Updated PDA account reads to use the codec to encode seeds in params

5ca4719

Removed string seed encoder

3584a6f

Reverted moving ChainWriter helper methods to utils

18975c9

amit-momin temporarily deployed to integration January 15, 2025 20:50 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 15, 2025 20:51 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 15, 2025 20:56 — with GitHub Actions Inactive

ilija42 reviewed Jan 15, 2025

View reviewed changes

Updated the PDA codec entry to use the existing builder methods

35b2596

amit-momin had a problem deploying to integration January 15, 2025 23:58 — with GitHub Actions Error

amit-momin temporarily deployed to integration January 15, 2025 23:58 — with GitHub Actions Inactive

amit-momin had a problem deploying to integration January 15, 2025 23:58 — with GitHub Actions Failure

Merged the pda read binding with the existing account read binding

67a24e1

amit-momin temporarily deployed to integration January 16, 2025 18:46 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 16, 2025 18:47 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 16, 2025 18:52 — with GitHub Actions Inactive

amit-momin changed the title ~~[WIP] Enable ChainReader to read PDA account state~~ Enable ChainReader to read PDA account state Jan 16, 2025

amit-momin marked this pull request as ready for review January 16, 2025 18:57

amit-momin requested a review from a team as a code owner January 16, 2025 18:57

Merge branch 'develop' into NONEVM-983-Implement-PDA-Seed-hasher

75e069c

amit-momin had a problem deploying to integration January 17, 2025 16:01 — with GitHub Actions Failure

amit-momin temporarily deployed to integration January 17, 2025 16:01 — with GitHub Actions Inactive

ilija42 approved these changes Jan 17, 2025

View reviewed changes

amit-momin had a problem deploying to integration January 17, 2025 16:41 — with GitHub Actions Failure

EasterTheBunny approved these changes Jan 17, 2025

View reviewed changes

amit-momin had a problem deploying to integration January 17, 2025 17:28 — with GitHub Actions Failure

amit-momin temporarily deployed to integration January 17, 2025 19:42 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 17, 2025 19:46 — with GitHub Actions Inactive

Merge branch 'develop' into NONEVM-983-Implement-PDA-Seed-hasher

23ee58d

amit-momin temporarily deployed to integration January 17, 2025 19:58 — with GitHub Actions Inactive

amit-momin temporarily deployed to integration January 17, 2025 20:04 — with GitHub Actions Inactive

aalu1418 approved these changes Jan 17, 2025

View reviewed changes

aalu1418 merged commit 933f88f into develop Jan 17, 2025
35 of 36 checks passed

aalu1418 deleted the NONEVM-983-Implement-PDA-Seed-hasher branch January 17, 2025 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ChainReader to read PDA account state #1003

Enable ChainReader to read PDA account state #1003

amit-momin commented Jan 7, 2025 •

edited

Loading

silaslenihan left a comment

silaslenihan Jan 8, 2025

amit-momin Jan 8, 2025

ilija42 Jan 15, 2025

amit-momin Jan 15, 2025

ilija42 Jan 15, 2025

amit-momin Jan 15, 2025

ilija42 Jan 15, 2025 •

edited

Loading

amit-momin Jan 15, 2025

ilija42 Jan 15, 2025

ilija42 Jan 15, 2025 •

edited

Loading

ilija42 Jan 15, 2025 •

edited

Loading

ilija42 left a comment

EasterTheBunny left a comment

cl-sonarqube-production bot commented Jan 17, 2025

Enable ChainReader to read PDA account state #1003

Enable ChainReader to read PDA account state #1003

Conversation

amit-momin commented Jan 7, 2025 • edited Loading

silaslenihan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilija42 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilija42 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

ilija42 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

ilija42 left a comment

Choose a reason for hiding this comment

EasterTheBunny left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Jan 17, 2025

Quality Gate failed

amit-momin commented Jan 7, 2025 •

edited

Loading

ilija42 Jan 15, 2025 •

edited

Loading

ilija42 Jan 15, 2025 •

edited

Loading

ilija42 Jan 15, 2025 •

edited

Loading