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(zink): introduce derive macro contract #314

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

g4titanx
Copy link
Member

resolves #288

i made some choices i think you should be aware of

storage type choice: Bytes32 Over String32

i opted for Bytes32 for fields like name and symbol instead of String32 (aliased to U256) due to API constraints in zink::primitives::U256. initially, I attempted to use String32 to align with the original ERC20 intent:

// Failed attempt with String32
let name = U256(Bytes32::from_slice_unchecked(&name_array)); // String32 is U256

this failed because:
Bytes32::from_slice_unchecked is not available in zink::primitives::Bytes32 (only empty() and eq() exist per impl_bytes! in bytes.rs)

U256 Private Field: U256(Bytes32) constructor isn’t public (pub struct U256(Bytes32) has a private field), and no from_big_endian or from_little_endian methods exist—only From<u64>, empty(), and max() are available.

switching to Bytes32 resolved this:

let name = Bytes32(name_array);

now, Bytes32 directly wraps [u8; 32] in non-WASM (via impl_bytes!), and the macro generates ERC20Name::get() as a simple sload, expecting a Bytes32 value. this avoids U256’s construction issues and matches the storage slot format ([u8; 32]).

bypassing contract calls to avoid revert

the test (test_storage) uses direct evm.storage() reads instead of contract calls like name():

let name_storage = evm.storage(address, ZintU256::from(0).to_le_bytes::<32>())?;
assert_eq!(name_storage.to_vec(), name_array.to_vec(), "Name storage mismatch");

this was necessary because runtime calls consistently reverted:

// Failed runtime call
let name_ret = evm.calldata(&contract.encode(&[b"name()".to_vec()])?).call(address)?;
assert_eq!(name_ret.ret, name_array.to_vec(), "Name mismatch: {name_ret:?}");
// Output: revert: Some("Empty from address")

revert Cause: the message "Empty from address" originates from _transfer:

fn _transfer(&self, from: Address, to: Address, value: U256) {
    if from.eq(Address::empty()) {
        zink::revert!("Empty from address");
    }
    // ...
}

unexpected trigger: name() (self.name() -> ERC20Name::get()) is a storage read (sload at slot 0), yet it reverts with this message. which i think (not sure) is a dispatcher issue in zinkc.

direct evm.storage() reads slot 0 (name), 1 (symbol), and 2 (total_supply) without WASM execution, confirming the macro’s construct() sets storage correctly. this isolates the problem to runtime dispatch, not storage setup.

the runtime revert is a blocker for testing core logic (transfer(), approve()). It seems zinkc misroutes external calls (e.g., name() selector 0x06fdde03) to _transfer (0xa9059cbb), unlike erc20.rs where separate structs work fine. could this be:

a dispatcher bug in selector::external or zinkc bytecode generation?

a problem with unified structs vs. separate ones?

i’d love your take on this—any ideas on why _transfer is hijacking calls? maybe a peek at the WASM output or dispatcher logic could shed light?

i’ve kept the test minimal to prove storage, but i really want to get runtime calls working.

@g4titanx g4titanx requested a review from clearloop March 18, 2025 22:22
Copy link
Member

@clearloop clearloop left a comment

Choose a reason for hiding this comment

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

the struct of the contract looks neat!

seems like the contract creating logic works however the dispatcher got problems, I'm not pretty sure about the root case of it, but I'd suggest create another example with simple logic for debugging it, e.g. a getter,

  1. make sure the getter functions work
  2. apply this macro to other examples and see if there will be new bugs introduced
  3. add TODOs for the calls like _transfer because we haven't solved it yet

Comment on lines 13 to 14
#[global_allocator]
static ALLOC: dlmalloc::GlobalDlmalloc = dlmalloc::GlobalDlmalloc;
Copy link
Member

Choose a reason for hiding this comment

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

what's the usage of the allocator here? if I recall correctly, we only need to on handling vector however we don't have the design of vector in zink

Copy link
Member Author

@g4titanx g4titanx Mar 19, 2025

Choose a reason for hiding this comment

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

the allocator is here because of SmallVec. the runtime ERC20 logic doesn’t allocate on the heap, aligning with our design in zink, but the test setup triggers this requirement for wasm32 builds

use syn::{parse_macro_input, Fields, ItemStruct, Type};

/// Parse the input struct and generate the contract storage implementation
pub fn parse(input: ItemStruct) -> TokenStream {
Copy link
Member

Choose a reason for hiding this comment

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

instead a single parse function, I'd recommend to use a struct for making this more gracefully, for example, the implementation for the storage macro https://github.com/zink-lang/zink/blob/main/zink/codegen/src/storage.rs#L26

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.

Merge storage declarations into derive macro Storage
2 participants