Skip to content
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

Feat/pallet anchor #436

Merged
merged 13 commits into from
Sep 19, 2024
Merged

Feat/pallet anchor #436

merged 13 commits into from
Sep 19, 2024

Conversation

ivan-cholakov
Copy link
Contributor

@ivan-cholakov ivan-cholakov commented Sep 19, 2024

Proposed changes

Functionality for anchoring to the avn chain, encapsulated in a single pallet

Type of change/Merge

🚨What type of change is this PR?

Put an x in the boxes that apply

  • Release
    • Increase versions
    • Baseline tests passed
    • Release type:
      • Major release
      • Minor release
      • Patch release

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR.

  • You describe the purpose of the PR, e.g.:
    • What does it do?
    • Highlight what important points reviewers should know about;
    • Indicates if there is something left for follow-up PRs.
  • Documentation updated
  • Business logic tested successfully
  • Verify First, Write Last: In Substrate development, it is important that you always ensure preconditions are met and return errors at the beginning. After these checks have completed, then you may begin the function's computation.

Further comments

ivan-cholakov and others added 8 commits September 19, 2024 14:12
Signed-off-by: Ivan Cholakov <icholakov1@gmail.com>
Co-authored-by: Nahu <39748285+nahuseyoum@users.noreply.github.com>
Signed-off-by: Ivan Cholakov <icholakov1@gmail.com>
Co-authored-by: nahuseyoum <nahu.seyoum@aventus.io>
Co-authored-by: Nahu <39748285+nahuseyoum@users.noreply.github.com>
runtime/avn/src/lib.rs Outdated Show resolved Hide resolved
pallets/avn-anchor/src/lib.rs Show resolved Hide resolved
pallets/avn-anchor/src/lib.rs Outdated Show resolved Hide resolved
});
}

// #[test]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test being kept?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be ok to have multiple chain names because the handler and chainId will be different so we can remove this test. But I would prefer to merge as is and then do a separate PR to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why check in, commented out code?

impl InnerCallValidator for TestAvnProxyConfig {
type Call = RuntimeCall;

fn signature_is_valid(call: &Box<Self::Call>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need Box? Why not just &Self::Call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the size of Call is unknown (it can be any call to the runtime) and boxing it gives it a known size (the size of the pointer to the heap)

impl<T: Config> InnerCallValidator for Pallet<T> {
type Call = <T as Config>::RuntimeCall;

fn signature_is_valid(call: &Box<Self::Call>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

@ivan-cholakov ivan-cholakov merged commit f3c548c into main Sep 19, 2024
5 checks passed
@ivan-cholakov ivan-cholakov deleted the feat/pallet-anchor branch September 19, 2024 17:01
Copy link
Contributor

@thadouk thadouk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR title needs updating


pub type ChainId = u32;
#[derive(Clone, Encode, Decode, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
#[scale_info(skip_type_params(T))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this?


#[pallet::storage]
#[pallet::getter(fn checkpoints)]
pub type Checkpoints<T: Config> = StorageDoubleMap<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the benefit of having different Nonce/CheckpointID per chain? wouldn't a universal nonce for all checkpoints be simpler?

});
}

// #[test]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why check in, commented out code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this intended to check in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants