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(storage) : add transient storage for cancun #292

Merged
merged 24 commits into from
Dec 3, 2024

Conversation

malik672
Copy link
Contributor

@malik672 malik672 commented Nov 30, 2024

Resolves #260

0x30bE4D758d86cfb1Ae74Ae698f2CF4BA7dC8d693

Changes

  • Support transient storage introduced in Cancun, TSTORE (0x5D) and TLOAD (0x5D)
  • add test for transient storage
  • example for transient storage

@malik672
Copy link
Contributor Author

@clearloop

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.

your approach is correct! however this PR is incomplete, please request for review at least after passing the CI or it would be inefficient for requesting changes to fix the CI, on the other hand, feel free to request for review if you want to identify if you are in the right direction ( the answer is yes for the current PR! )

  1. CI is not passed
  2. need to merge the transient storage with storage in the token parser in codegen ( add a new field to Storage for identify if the passed struct is transient or not )
  3. missing handlers in the host function registry https://github.com/zink-lang/zink/blob/main/codegen/src/wasm/host.rs

examples/transient_storage.rs Outdated Show resolved Hide resolved
tests/transient_storage.rs Outdated Show resolved Hide resolved
zink/src/ffi/evm.rs Outdated Show resolved Hide resolved
zink/codegen/src/transient_storage.rs Outdated Show resolved Hide resolved
@malik672 malik672 requested a review from clearloop December 1, 2024 06:57
@malik672
Copy link
Contributor Author

malik672 commented Dec 1, 2024

@clearloop

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.

Sorry for the late! Everything looks good now! Please fix the CI 🙏

In case you don't know how to process tests locally

# test all tests
cargo tt 

# build all examples
cargo be

# test all examples
cargo te

# run clippy to all packages
cargo cc

codegen/src/wasm/host.rs Outdated Show resolved Hide resolved
codegen/src/wasm/host.rs Outdated Show resolved Hide resolved
tests/transient_storage.rs Outdated Show resolved Hide resolved
examples/transient_storage.rs Show resolved Hide resolved
@clearloop clearloop mentioned this pull request Dec 2, 2024
4 tasks
@malik672 malik672 requested a review from clearloop December 2, 2024 07:54
@malik672
Copy link
Contributor Author

malik672 commented Dec 2, 2024

@clearloop i'm missing something with the exports

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.

we don't need HostFunc::TransientOp since it is actually HostFunc::Evm now ^ ^

codegen/src/wasm/host.rs Outdated Show resolved Hide resolved
codegen/src/wasm/host.rs Outdated Show resolved Hide resolved
codegen/src/wasm/host.rs Outdated Show resolved Hide resolved
@malik672
Copy link
Contributor Author

malik672 commented Dec 2, 2024

we don't need HostFunc::TransientOp since it is actually HostFunc::Evm now ^ ^

yeah true but the inability of the example to not find the transient macro is suprising

@malik672 malik672 requested a review from clearloop December 2, 2024 09:29
@malik672
Copy link
Contributor Author

malik672 commented Dec 2, 2024

@clearloop i think i need help here

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.

Just import-export issues ))

error[E0433]: failed to resolve: could not find `transient_storage` in `zink`
  --> examples/transient_storage.rs:10:1
   |
10 | #[zink::transient_storage(i32)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | |
   | could not find `transient_storage` in `zink`
   | help: a trait with a similar name exists: `TransientStorage`
   |
   = note: this error originates in the attribute macro `zink::transient_storage` (in Nightly builds, run with -Z macro-backtrace for more info)

this is not pointing to the proc-macro but the path zink::transient_storage::TransientStorage in your quote haha

zink/src/lib.rs Outdated Show resolved Hide resolved
zink/codegen/src/storage.rs Outdated Show resolved Hide resolved
malik672 and others added 2 commits December 2, 2024 14:47
@malik672 malik672 requested a review from clearloop December 2, 2024 14:10
@malik672
Copy link
Contributor Author

malik672 commented Dec 2, 2024

@clearloop transient storage is not present yet in revm cachedb

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.

@clearloop transient storage is not present yet in revm cachedb

Oh I saw it, because it is temp storage, in this case, we just need a set_get call to verify our implementation

examples/transient_storage.rs Outdated Show resolved Hide resolved
@malik672 malik672 requested a review from clearloop December 2, 2024 17:36
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.

You mapped the wrong opcodes for transient storage, please don't trust AI's outputs for the opcodes, comparing them with evm.codes please

evm/opcodes/src/cancun.rs Outdated Show resolved Hide resolved
evm/opcodes/src/cancun.rs Show resolved Hide resolved
@malik672 malik672 requested a review from clearloop December 3, 2024 05:46
@malik672
Copy link
Contributor Author

malik672 commented Dec 3, 2024

@clearloop done

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.

@clearloop clearloop merged commit f4ada38 into zink-lang:main Dec 3, 2024
4 checks passed
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.

Transient storage for contracts
2 participants