wormhole_attester: per-symbol PDAs for attestation state #567

drozdziak1 · 2023-02-06T15:19:08Z

This change replaces the big attestation state PDA with individual per-symbol ones. This ensures more predictable tx costs and is generally cheaper. The change is achieved by reusing the product accounts on the attest() endpoint and putting the state PDAs in their place.

This change bumps price batch format to v3.1 with a new backwards compatible field - prev_attestation_time. This is the last time we've successfully attested the price. If no prior record exists, the current time is used (the same as attestation_time). The new field is backed by a new PDA for the attester contract, called 'attestation state'. In this PDA, we store a Pubkey -> Metadata hashmap for every price. Currently, the metadata stores just the latest successful attestation timestamp for use with the new field.

…ttester-prev-timestamp-field

vercel · 2023-02-06T15:19:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated
example-oracle-amm	⬜️ Ignored (Inspect)	Visit Preview		Feb 8, 2023 at 3:35PM (UTC)
xc-admin-frontend	⬜️ Ignored (Inspect)	Visit Preview		Feb 8, 2023 at 3:35PM (UTC)

Using Option<> for this makes fallback to latest value more convenient

ali-behjati · 2023-02-06T16:36:16Z

wormhole_attester/program/src/attest.rs

+    pub pyth_state: AttestationStatePDA<'b>,
+    pub pyth_price: Info<'b>,

-    pub pyth_product2: Option<Info<'b>>,
-    pub pyth_price2:   Option<Info<'b>>,
+    pub pyth_state2: Option<AttestationStatePDA<'b>>,
+    pub pyth_price2: Option<Info<'b>>,

-    pub pyth_product3: Option<Info<'b>>,
-    pub pyth_price3:   Option<Info<'b>>,
+    pub pyth_state3: Option<AttestationStatePDA<'b>>,
+    pub pyth_price3: Option<Info<'b>>,

-    pub pyth_product4: Option<Info<'b>>,
-    pub pyth_price4:   Option<Info<'b>>,
+    pub pyth_state4: Option<AttestationStatePDA<'b>>,
+    pub pyth_price4: Option<Info<'b>>,

-    pub pyth_product5: Option<Info<'b>>,
-    pub pyth_price5:   Option<Info<'b>>,
+    pub pyth_state5: Option<AttestationStatePDA<'b>>,
+    pub pyth_price5: Option<Info<'b>>,



I guess you need to make the states mutable here

ali-behjati

lgtm, i'll approve once the CI works. It seems like a mutability issue that I think is related to considering the state accounts mutable (on the contract)

guibescos · 2023-02-06T20:26:15Z

wormhole_attester/program/src/attest.rs

+                state.0.info(),
+                accs.payer.key,
+                solitaire::CreationLamports::Exempt,
+                state_serialized.len(),


Instead of using state_serialized.len(), you can create a constant AttestationState::LEN which is equal to the maximum size the account can have. Then you can deleting all the resizing code.

Or turn last_attested_trading_publish_time into a Timestamp instead of an Option (the comment below) and use the AccountSize solitaire trait.

I think I prefer spending the extra gas in exchange for not maintaining a constant for this. I expect us to want more metadata in this state at some point, if we don't drop the attester in the future. I think without the realloc logic it can live in the uninitialized if branch, so this is only done once in a PDA's lifetime ( that's the case in the change that removes the unneeded realloc()). Fundamentally, I think Solitaire could benefit from a flag that triggers this call for uninitialized data accounts. On persist() it serializes the struct anyway.

guibescos · 2023-02-06T21:20:08Z

wormhole_attester/program/src/attestation_state.rs

 pub struct AttestationState {
    /// The last trading publish_time this attester saw
-    pub last_attested_trading_publish_time: UnixTimestamp,
+    pub last_attested_trading_publish_time: Option<UnixTimestamp>,


I don't think you need this to be an option.
Because the option logic is already captured in whether the account is initialized or not.
i.e. if the account doesn't exist it's a none
if it exists it's a some

That makes sense. Changed to just look at is_initialized

guibescos · 2023-02-06T21:20:43Z

wormhole_attester/program/src/attestation_state.rs

-    Data<'b, AttestationStateMap, { AccountState::MaybeInitialized }>,
-    "p2w-attestation-state-v1",
->;
+impl<'a, 'b: 'a> Peel<'a, 'b> for AttestationStatePDA<'b> {


Surprised you have to derive peel and persist manually. Why can't solitaire handle this?

tl;dr that's because automating this in a derive macro is surprisingly difficult and Solitaire cannot do this currently

Going on a bit of a framework design tangent, it's actually a very interesting topic. I think a solid dedicated derive(Peel) could make this go away. The thing with Solana account deserialization is that aimost any attempt at a framework will focus on adding behavior modifiers on some sort of user accounts struct, one way or another. Anchor chose to go with macro attributes as the first class citizen, where you just specify requirements in the spirit of ETH modifiers, onto struct members as part of the derive macro. Solitaire went a different direction to do as little simple codegen as necessary. The idea being to contain as much of the validation logic in regular Rust code instead of the derive macro. So the way to enforce specific rules is abstracted away from the derive macro (Notice how it says FromAccounts and not Peel). The macro only knows that all members should implement Peel and it just calls that on each account in the incoming slice on-chain. Thus most automated validation comes from stacking types like Mut<Derive<Data<MyDataStruct, Initialized>>>, because they implement Peel and try to cover most simple cases. There are several unanticipated problems with our approach.First,t Solana's Rust safety choices make it very hard to come up with easy to use generics (we have to use lifetimes like 'a, 'b and so on). Also, our struct-based strategy that makes very little info available to the derive macro means that relationships between account struct members are almost impossible to express statically in struct definition

AHHHH ok I think I get what's the problem

You can get away with this by making :
pub type AttestationStatePDA<'b> =Data<'b, AttestationState, { AccountState::MaybeInitialized }>
instead of :

pub struct AttestationStatePDA<'b>( pub Data<'b, AttestationState, { AccountState::MaybeInitialized }>, );

I think this is just a strict simplification of the code

That's exactly what I tried to do before I needed to add the Seeded impl. However, It is not possible to implement foreign traits for foreign types in Rust :( In this case, the typedef is considered equivalent to the foreign type we assign to it, so the compiler yells, because the trait is also foreign.

guibescos

Some comments

ali-behjati

🔥

jayantk

i'm happy if @ali-bahjati and @guibescos are

jayantk · 2023-02-07T19:18:38Z

wormhole_attester/program/src/attest.rs

+        // Serialize the state to calculate rent/account size adjustments
+        let state_serialized = state.0 .0 .1.try_to_vec()?;
+
+        if state.0 .0.is_initialized() {


extract a helper function for all of this account allocation logic

I think this realloc logic needs to go away. This account is fixed sized. It should never get realloc and this code is too complex.

You're right, this made sense only for the large PDA. Thanks.

drozdziak1 added 5 commits January 18, 2023 15:54

wormhole-attester: Use publish_time instead of attestation_time

c0d8cb0

wormhole_attester: use prev_publish_time for non-trading prices

cb72921

wormhole_attester: per-symbol state PDAs, stop using prod accounts

c044f65

Merge remote-tracking branch 'origin/main' into drozdziak1/wormhole-a…

8c390c9

…ttester-prev-timestamp-field

attester: Use Option to detect if previous state exists

88da084

Using Option<> for this makes fallback to latest value more convenient

drozdziak1 requested review from ali-behjati, guibescos and jayantk February 6, 2023 15:50

drozdziak1 changed the title ~~[WIP]: wormhole_attester: per-symbol PDAs for attestation state~~ wormhole_attester: per-symbol PDAs for attestation state Feb 6, 2023

wormhole_attester: client AccountMeta typo

b8b029e

ali-behjati reviewed Feb 6, 2023

View reviewed changes

guibescos reviewed Feb 6, 2023

View reviewed changes

drozdziak1 added 2 commits February 7, 2023 11:50

wormhole_attester: fix mutability error

13903bc

wormhole_attester: stop using Option<> for on-chain state

bef9a46

ali-behjati approved these changes Feb 7, 2023

View reviewed changes

jayantk reviewed Feb 7, 2023

View reviewed changes

wormhole_attester: remove unused realloc logic for attestation state

c8a3b57

drozdziak1 merged commit d52f4d4 into main Feb 8, 2023

drozdziak1 deleted the drozdziak1/wormhole-attester-prev-timestamp-field branch February 8, 2023 16:04

guibescos approved these changes Feb 8, 2023

View reviewed changes

wormhole_attester: per-symbol PDAs for attestation state #567

wormhole_attester: per-symbol PDAs for attestation state #567

Uh oh!

Conversation

drozdziak1 commented Feb 6, 2023

Uh oh!

vercel bot commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment

Choose a reason for hiding this comment

Uh oh!

guibescos Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guibescos Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drozdziak1 Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guibescos Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drozdziak1 Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guibescos left a comment

Choose a reason for hiding this comment

Uh oh!

ali-behjati left a comment

Choose a reason for hiding this comment

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guibescos Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vercel bot commented Feb 6, 2023 •

edited

Loading

guibescos Feb 6, 2023 •

edited

Loading

guibescos Feb 6, 2023 •

edited

Loading

drozdziak1 Feb 8, 2023 •

edited

Loading

guibescos Feb 6, 2023 •

edited

Loading

drozdziak1 Feb 7, 2023 •

edited

Loading

guibescos Feb 7, 2023 •

edited

Loading