Skip to content

docs: fix stack comments in counter.masm - #244

Open
Mustdzyl wants to merge 1 commit into
0xMiden:mainfrom
Mustdzyl:fix/counter-masm-stack-comments
Open

docs: fix stack comments in counter.masm#244
Mustdzyl wants to merge 1 commit into
0xMiden:mainfrom
Mustdzyl:fix/counter-masm-stack-comments

Conversation

@Mustdzyl

Copy link
Copy Markdown

counter.masm is the first contract most people read, and its stack comments don't match what the procedures actually leave on the stack.

Per the protocol docstrings:

  • active_account::get_itemInputs: [slot_id_suffix, slot_id_prefix]Outputs: [VALUE], a word.
  • native_account::set_itemInputs: [slot_id_suffix, slot_id_prefix, VALUE]Outputs: [OLD_VALUE], also a word.

And sys::truncate_stack only removes elements below the top 16 ("the top 16 elements of the stack remain unchanged"), so it doesn't clear OLD_VALUE either.

That makes two comments in increment_count wrong rather than merely abbreviated: it is documented as Outputs: [] and # => [], but it actually leaves the previous count on the stack. get_count is documented as returning [count] where get_item returns a full word.

The other account modules in this repo already get this right — count_reader.masm documents # => [OLD_VALUE, pad(12)] and mapping_example_contract.masm documents # => [OLD_VALUE] / # => [VALUE] — so this just brings counter.masm in line with its siblings.

Comments only; no behavioural change.

One thing I deliberately left alone: increment_count returning OLD_COUNT looks incidental rather than intended. If it should return nothing, a dropw after set_item would do it — happy to add that instead if you'd prefer the code change over the comment change.

get_item returns a word and set_item returns the previous value, so the
documented stack effects in counter.masm were inaccurate. The other account
modules in this repo already document these correctly.
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.

1 participant