Implement SMO instruction
#159
Conversation
|
Blocked by #158 |
vlopes11
force-pushed
the
vlopes11/smo
branch
2 times, most recently
from
8687140 to
1cbeb96
Compare
vlopes11
requested review from
Voxelot,
adlerjohn,
bvrooman,
Dentosal,
pixelcircuits and
rakita
|
|
||
| let fp = self.registers[REG_FP] as usize; | ||
| let txid = self.tx_id(); | ||
| let data = a as usize + Address::LEN; |
There was a problem hiding this comment.
Maybe this should use Bytes32::LEN like on line 316 for consistency.
There was a problem hiding this comment.
True, they should be the same. The concrete type is in fact Address, instead of raw bytes. Better to use that so its clear what is expected of this register:
| @@ -88,20 +88,25 @@ impl<S> Interpreter<S> { | |||
|
|
|||
| // compute the initial free balances for each asset type | |||
| pub(crate) fn initial_free_balances(&self) -> Result<HashMap<AssetId, Word>, InterpreterError> { | |||
| let base_asset = AssetId::default(); | |||
There was a problem hiding this comment.
Orthogonal to this PR, but I'm thinking the base asset's ID should be a parameter in the VM. It's not necessarily guaranteed to be AssetId::default() always. Both the specs and the implementation should be modular in this regard. Can you file an issue to track changing?
| @@ -52,7 +52,7 @@ impl<S> Interpreter<S> { | |||
| .tx | |||
| .witnesses() | |||
| .get(b as usize) | |||
| .ok_or(PanicReason::OutputNotFound) | |||
| .ok_or(PanicReason::WitnessNotFound) | |||
There was a problem hiding this comment.
This looks like an orthogonal bugfix. Ideally make a separate PR for this.
There was a problem hiding this comment.
Thats true, I just spotted this nit while debugging
There was a problem hiding this comment.
Looks fine to me, would like @pixelcircuits and @Voxelot to give a review as well.
No description provided.