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

refactor: build chain extension method #121

Merged

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Jul 23, 2024

Link to discussion: #71 (comment)

Contract size: Original wasm size: 31.2K, Optimized: 7.5K

pub(crate) fn build_extension_method(
	version: u8,
	function: u8,
	module: u8,
	dispatchable: u8,
) -> ChainExtensionMethod<(), (), (), false> {
	ChainExtensionMethod::build(u32::from_le_bytes([version, function, module, dispatchable]))
}

@chungquantin chungquantin changed the base branch from main to daan/feat-assets July 23, 2024 16:37
@evilrobot-01 evilrobot-01 changed the title refractor: build chain extension method refactor: build chain extension method Jul 23, 2024
@evilrobot-01
Copy link
Collaborator

An alternative implementation which keeps things simpler imo, taking advantage of scopes: ee4d499

  • reuses existing lib.rs rather than introducing a new module for a single function. build_extension_method will probably be used in almost every submodule, so easier if its globally available rather than within a separate utils module.
  • adds a build_extension_method at the v0 module root, so we dont have to repeat V0 everywhere within the module
  • adds simple build_dispatch and build_read_state functions as well, to avoid having to repeat the DISPATCH and READ_STATE constants throughout all the implemented functions. We could drop the build_* prefix on these functions too.
  • no pub(crate) modifiers required

@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Jul 23, 2024

Could you also add the contract size in the description of the PR after the changes made (with --release)

@evilrobot-01 evilrobot-01 mentioned this pull request Jul 23, 2024
3 tasks
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Looking good, think there are a few final possible improvements and then should be good to merge.

pop-api/src/lib.rs Outdated Show resolved Hide resolved
pop-api/src/lib.rs Outdated Show resolved Hide resolved
pop-api/src/v0/assets/mod.rs Outdated Show resolved Hide resolved
pop-api/src/v0/assets/mod.rs Outdated Show resolved Hide resolved
pop-api/src/v0/mod.rs Outdated Show resolved Hide resolved
pop-api/src/v0/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Looking nice and clean! One final improvement suggested.

pop-api/src/v0/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your patience! This will be a very important piece to everything that needs to be built out, so great to have such a tidy implementation.

@chungquantin chungquantin changed the base branch from daan/feat-assets to daan/api July 26, 2024 09:01
@Daanvdplas
Copy link
Collaborator

I think something went wrong when merging 'daan/api' because the helper functions are now not being used in the fungibles apis.

@chungquantin
Copy link
Collaborator Author

@Daanvdplas Rebased. Should be ok now. Can you check it again?

@Daanvdplas Daanvdplas merged commit 027cb85 into r0gue-io:daan/api Jul 26, 2024
5 checks passed
@Daanvdplas
Copy link
Collaborator

Good work @chungquantin!

@chungquantin chungquantin deleted the chungquantin/refractor-feat-assets branch July 26, 2024 17:36
chungquantin added a commit that referenced this pull request Sep 6, 2024
Co-authored-by: Daan van der Plas <[email protected]>
Co-authored-by: Daanvdplas <[email protected]>
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.

3 participants