Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Implementing POP and MSTORE traits for Bus Mapping #140

Closed
wants to merge 6 commits into from
Closed

Implementing POP and MSTORE traits for Bus Mapping #140

wants to merge 6 commits into from

Conversation

rahulbs98
Copy link

@rahulbs98 rahulbs98 commented Oct 25, 2021 •
edited
Loading

  • Check pop and confused about pop_op
  • Yet to do MSTORE

Co-authored-by: CPerezz c.perezbaro@gmail.com

@github-actions github-actions bot added the crate-bus-mapping Issues related to the bus-mapping workspace member label Oct 25, 2021
@rahulbs98 rahulbs98 changed the title Introduce a global Context for bus-mapping (#134) Implementing POP opcode for Bus Mapping Oct 25, 2021
@rahulbs98 rahulbs98 linked an issue Oct 25, 2021 that may be closed by this pull request
bus-mapping: Implement Opcode trait for all non-storage-related OpcodeIds #83
Closed
78 tasks
@rahulbs98 rahulbs98 self-assigned this Oct 25, 2021
@rahulbs98 rahulbs98 changed the title Implementing POP opcode for Bus Mapping Implementing POP and MSTORE traits for Bus Mapping Oct 25, 2021
@CPerezz CPerezz mentioned this pull request Oct 25, 2021
Merged
CPerezz
CPerezz reviewed Oct 25, 2021
View reviewed changes
bus-mapping/src/evm/opcodes/pop.rs Show resolved Hide resolved
bus-mapping/src/evm/opcodes/pop.rs Outdated Show resolved Hide resolved
CPerezz
CPerezz suggested changes Oct 30, 2021
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

I think this is the last change required. Once done, try to run make test-all locally to spot any docs/ clippy errors. And we will trigger the CI :)

bus-mapping/src/evm/opcodes/pop.rs Outdated Show resolved Hide resolved
CPerezz
CPerezz suggested changes Oct 30, 2021
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Still some inconsistencies between the trace you're generating in the tests and the things you create in the tests.

bus-mapping/src/evm/opcodes/pop.rs Outdated Show resolved Hide resolved
bus-mapping/src/evm/opcodes/pop.rs Outdated Show resolved Hide resolved
bus-mapping/src/evm/opcodes/pop.rs
.stack()
.deref()
.last()
.cloned()
Copy link
Member

Choose a reason for hiding this comment

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

Try copied. It might work since EvmWord is an array and should be copiable.

Copy link
Author

Choose a reason for hiding this comment

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

I still didnt understand this

@CPerezz
Copy link
Member

CPerezz commented Dec 17, 2021

Superset by current master.
Closing this for now.

@CPerezz CPerezz closed this Dec 17, 2021
@CPerezz CPerezz deleted the BusMapping_POP branch December 17, 2021 10:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CPerezz CPerezz Awaiting requested review from CPerezz

Assignees

@rahulbs98 rahulbs98

Labels
crate-bus-mapping Issues related to the bus-mapping workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bus-mapping: Implement Opcode trait for all non-storage-related OpcodeIds
2 participants
@rahulbs98 @CPerezz