Extend Address class to handle BN

[cgewecke]

(This topic came up in discussions last week...opening to get feedback on what the best approach might be.)

Background

• @ryanio noted that it would be good to manage addresses as Address everywhere in the VM.
• @cgewecke noted they're stored as BN on the vm stack and require several steps to convert to Buffer or String

Details

• addresses convert to BN simply: new BN(addressBuffer).
• when they are converted from BN, they are "anded" through a mask (to enforce width?)

   export function addressToBuffer(address: BN): Buffer {
      return address.and(MASK_160).toArrayLike(Buffer, 'be', 20)
   }

Question

Should the Address class be extended here to handle this case? Or should that logic stay in the VM (because it's an atypical usage).

Pro: cleaner to write new Address(addressBN).toString() than what is currently necessary at the VM
Con: a little weird? would also require updating this dependency in the monorepo

[ryanio]

Thanks for opening an issue to discuss :)

Yeah I still don't understand why the address would be stored as a BN as opposed to buffer. Would it be trivial to update the vm to handle it as a buffer? The AND mask cool, but are there any examples of a case where it's actually helping do something?

If we did want to introduce instantiating from a BN to Address, then I would suggest a static factory method Address.fromBN(value: BN) in the spirit of our recent refactorings in tx, block, etc. to keep the base constructor simple.

[cgewecke]

@ryanio

I still don't understand why the address would be stored as a BN as opposed to buffer

Not sure but would guess that BN was chosen as a default stack format because quantities are common and many opcodes perform arithmetic operations. By contrast there are 7 places in opcodes/functions.ts where an address is converted from BN to buffer.

Would it be trivial to update the vm to handle it as a buffer?

I think it would be simpler to keep a single stack data format. Otherwise wherever BN is assumed on pop we might need to add a check. The number of places expecting an address seems like a smaller set.

are there any examples of a case where [the AND mask is] actually helping do something

Good question - fooled around with this a bit and couldn't see any difference between masked and unmasked conversions. Unfortunately I can't find anything in the repo history about the original intent behind this either. So idk...is maybe a case of Chesterton's fence? What is your view?

I would suggest a static factory method Address.fromBN(value: BN)

Ok yes, that's smart.

[cgewecke]

Maybe the best thing to do is just extend the Address type at the VM for this use case...

[jochem-brouwer]

Small note here: the and strips off any leading characters in case you input a BN which is larger than the highest address. So the use case here is basically that it strips the BN "Buffer" of any leading numbers in case the Buffer is > 20 bytes.

[cgewecke]

@jochem-brouwer Ah thanks!! That's great.

[holgerd77]

Some reference: the stack item Buffer -> BN change has been made here ethereumjs/ethereumjs-monorepo#159 with performance being cited as the main reason for the switch.

Regarding AND mask: this is some perfect example for extremely implicit knowledge on code parts which needs to be digged out with great effort if not documented, it will be really valuable I would say if we put some continued attention on this and keep improving docs on such parts.

Since these things are so tiny (no one will likely open a PR just for this): would it make sense to have some ongoing static tracking issue with a TODO list where everyone can put such things with a short reference and everyone who feels like it takes these things up on a sideline along the dedicated main PR, just adds theses things and removes from the TODO list? 🤔

[cgewecke]

ethereumjs-vm 919 resolves this, closing.