Improve shared state locations and other stylistic improvements #698

chrismaree · 2024-10-28T11:21:22Z

WIP
WIP

Signed-off-by: chrismaree <christopher.maree@gmail.com>

socket-security · 2024-10-28T11:23:01Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

Signed-off-by: chrismaree <christopher.maree@gmail.com>

…om:across-protocol/contracts into chrismaree/improve-state-locations

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2024-10-28T13:20:20Z

programs/svm-spoke/src/instructions/fill.rs

    let state = &ctx.accounts.state;
    let current_time = get_current_time(state)?;

-    // Check if the fill status is filled


note that I changed the requires here! I think this check is redundant. the ONLY thing we care about is if the deposit is expired, not if it was filled (or not) to let us close the PDA. LMK if you'll agree.

right, and this could even allow us to close fill accounts that have been requested slow fill but failed to execute before the deadline.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Reinis-FRP · 2024-10-28T12:29:38Z

programs/test/src/lib.rs

    #[derive(Accounts)]
    pub struct Verify {}
    pub fn verify(ctx: Context<Verify>, root: [u8; 32], leaf: [u8; 32], proof: Vec<[u8; 32]>) -> Result<()> {
        let computed_root = process_proof(&proof, &leaf);


ctx parameter in verify above can be replaced with _ctx to avoid compile warnings

Reinis-FRP · 2024-10-28T13:11:29Z

programs/svm-spoke/src/instructions/testable.rs

 use anchor_lang::prelude::*;

-use crate::{ error::CustomError, state::State };
+use crate::{ error::SvmError, state::State };


nit: this still generates unused imports in test builds. Maybe we can avoid importing SvmError and instead reference it below using its full path crate::error::SvmError as it is used only in production builds?

ye, that's great. or conditionally import the file. I've changed it to just use crate::error::SvmError when referencing the error.

Reinis-FRP · 2024-10-28T13:21:50Z

programs/svm-spoke/src/instructions/admin.rs

+    error::SvmError,
    event::{
-        EmergencyDeletedRootBundle,
+        EmergencyDeleteRootBundle,


Shall we not better fix it in EVM as EmergencyDeletedRootBundle seems more consistent with other event names?

well then we break monitors and other things that depend on the EVM events rn, which is less ideal.

at least now they are consistent, which is better. I added a TODO to update both EVM and SVM.

Reinis-FRP · 2024-10-28T13:23:41Z

programs/svm-spoke/src/instructions/bundle.rs


-    #[account(
-        mut,
+    #[account(mut,


this seems bit off. Either we can fit all parameters in one line or each has to be on its own line

yes, you're right. I tried to see if I could fit all on one line & could not but messed up the formatting after the fact.

Reinis-FRP · 2024-10-28T13:40:55Z

programs/svm-spoke/src/error.rs

-pub enum CustomError {
-    #[msg("Only the owner can call this function!")]
-    NotOwner,
+pub enum SharedError {


nit: maybe CommonError would be a better name?

Reinis-FRP · 2024-10-28T14:11:40Z

programs/svm-spoke/src/instructions/refund_claims.rs

-    mint: Pubkey,
-    token_account: Pubkey
-) -> Result<()> {
+pub fn initialize_claim_account(ctx: Context<InitializeClaimAccount>, _: Pubkey, _: Pubkey) -> Result<()> {


we can delete the unused props here and rename them to _mint and _token_account in the outer function.

Reinis-FRP · 2024-10-28T14:13:56Z

programs/svm-spoke/src/instructions/slow_fill.rs

 pub fn request_v3_slow_fill(
    ctx: Context<SlowFillV3Relay>,
-    relay_hash: [u8; 32], // include in props, while not using it, to enable us to access it from the #Instruction Attribute within the accounts. This enables us to pass in the relay_hash PDA.
+    _: [u8; 32], // include in props, while not using it, to enable us to access it from the #Instruction Attribute within the accounts. This enables us to pass in the relay_hash PDA.


same comment as in others, rename it to _relay_hash in the outer call and not pass it to this inner function

Reinis-FRP · 2024-10-28T14:18:17Z

programs/svm-spoke/src/instructions/slow_fill.rs

 pub fn execute_v3_slow_relay_leaf(
    ctx: Context<ExecuteV3SlowRelayLeaf>,
-    relay_hash: [u8; 32],
+    _token_account: [u8; 32],


there is no token_account param in the outer call

Reinis-FRP · 2024-10-28T14:18:44Z

programs/svm-spoke/src/instructions/slow_fill.rs

+    _token_account: [u8; 32],
    slow_fill_leaf: V3SlowFill,
-    root_bundle_id: u32,
+    _: u32,


better to skip passing unused params from the outer call

Reinis-FRP · 2024-10-28T14:23:56Z

programs/svm-spoke/src/state/fill.rs

 }
+
+#[derive(AnchorSerialize, AnchorDeserialize, Clone)]
+pub struct V3RelayData {


maybe better to have this under something like src/common/v3_relay_data? then we could also move RefundAccount to src/common/refund_account as it also does not strictly belong to the state

ye, I think that's better. it's not really part of fill, and it's also not "state" in that it's not initialized. I'll update the way you've recommended.

I did not pull refund_account, though, as I think we might still end up changing this structure given the conversation we had last week where by we don't let the account be either TokenAccount OR ClaimAccount but rather pass in both of them to prevent relayers front running this set of methods.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Reinis-FRP · 2024-10-28T15:17:13Z

test/svm/SvmSpoke.Fill.ts

 const { recipient, initializeState, setCurrentTime, assertSE, assert } = common;

-describe("svm_spoke.fill", () => {
+describe.only("svm_spoke.fill", () => {


Suggested change

describe.only("svm_spoke.fill", () => {

describe("svm_spoke.fill", () => {

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Reinis-FRP

looking great!. just remove only mode in fill tests

chrismaree added 6 commits October 28, 2024 12:19

WIP

a9ad3ed

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/improve-state-locations

e3dffae

WIP

a47d060

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Discard changes to .prettierrc

7f2e8dc

Discard changes to package.json

b8b5588

WIP

a1aac56

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree added 4 commits October 28, 2024 12:23

Discard changes to yarn.lock

42e613b

WIP

e6abce0

Signed-off-by: chrismaree <christopher.maree@gmail.com>

git pushMerge branch 'chrismaree/improve-state-locations' of github.c…

f21443d

…om:across-protocol/contracts into chrismaree/improve-state-locations

WIP

703a14c

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree changed the title ~~Improve shared state locations~~ Improve shared state locations and other stylistic improvements Oct 28, 2024

chrismaree added 2 commits October 28, 2024 13:13

WIP

d39e92f

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

fe1777f

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree marked this pull request as ready for review October 28, 2024 12:50

chrismaree requested review from Reinis-FRP and md0x and removed request for Reinis-FRP October 28, 2024 13:18

chrismaree commented Oct 28, 2024

View reviewed changes

chrismaree added 2 commits October 28, 2024 14:28

WIP

9608327

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

bba86a5

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Reinis-FRP reviewed Oct 28, 2024

View reviewed changes

WIP

9ac1754

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Reinis-FRP reviewed Oct 28, 2024

View reviewed changes

WIP

66041a7

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Reinis-FRP approved these changes Oct 28, 2024

View reviewed changes

chrismaree merged commit 3cfd778 into master Oct 29, 2024
9 checks passed

chrismaree deleted the chrismaree/improve-state-locations branch October 29, 2024 10:48

	describe.only("svm_spoke.fill", () => {
	describe("svm_spoke.fill", () => {

Improve shared state locations and other stylistic improvements #698

Improve shared state locations and other stylistic improvements #698

Uh oh!

Conversation

chrismaree commented Oct 28, 2024

Uh oh!

socket-security bot commented Oct 28, 2024 • 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reinis-FRP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

socket-security bot commented Oct 28, 2024 •

edited

Loading