[Soroban] Linked support for block.number on soroban#1939
Conversation
| Some(address_var) | ||
| } | ||
| ast::Builtin::BlockNumber => { | ||
| let block_var_no = vartab.temp_anonymous(&Type::Uint(64)); |
There was a problem hiding this comment.
I prefer to use vartab.temp_name("block_number", ...)
it help us when we debug the CFG.
| //decoding to uint64 since get_ledger_sequence returns | ||
| let decoded_block_number = | ||
| soroban_decode_arg(block_var, cfg, vartab, ns, Some(Type::Uint(64))); | ||
| Some(decoded_block_number) |
There was a problem hiding this comment.
also here since we can directly pack the function call inside Some instead of using a variable it will be better.
|
We just need some minor changes. |
There was a problem hiding this comment.
Pull request overview
Adds Soroban support for block.number by wiring the Soroban host function for ledger sequence into the codegen lowering path, and validates behavior with a dedicated Soroban testcase. This aligns Soroban’s block.number semantics with Stellar’s ledger sequence number per issue #1937.
Changes:
- Add
HostFunctions::GetLedgerSequence("x.3") and declare it as an external Soroban host function. - Lower
ast::Builtin::BlockNumberinSorobanTarget::lower_builtinviaGetLedgerSequence+ Soroban value decoding touint64. - Add Soroban tests to exercise
block.numberacross multiple ledger sequence samples.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/soroban_testcases/mod.rs | Registers the new Soroban block number testcase module. |
| tests/soroban_testcases/block_number.rs | New test validating block.number maps to ledger sequence on Soroban. |
| src/emit/soroban/mod.rs | Declares the new Soroban host function signature and adds it to extern declarations. |
| src/codegen/targets/soroban/mod.rs | Lowers block.number via GetLedgerSequence and decodes to uint64. |
| src/codegen/mod.rs | Adds GetLedgerSequence to HostFunctions and maps it to "x.3". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| args: vec![], | ||
| }, | ||
| ); | ||
| //decoding to uint64 since get_ledger_sequence returns |
There was a problem hiding this comment.
i don't think we need to add comment here, code is short and clear.
There was a problem hiding this comment.
Thank you for the review
Lemme apply those changes and commit again. Gimme a min
There was a problem hiding this comment.
ping me when u update the pr
There was a problem hiding this comment.
@Islam-Imad I've made the changes accordingly
|
Let me check the changes and see where the code fails. I'll mitigate and update |
i think its github issue , i triggered the rerun for the failed job |
Signed-off-by: slash-aech <harshitbenke@proton.me>
removed comments, changed variable type and removed redudant variable returning Signed-off-by: slash-aech <harshitbenke@proton.me>
Signed-off-by: slash-aech <harshitbenke@proton.me>
|
thanks @slash-aech |
Closes #1937
Contents
Added following features from pull 1921
modified: src/codegen/mod.rs
modified: src/codegen/targets/soroban/mod.rs
added arm to
lower_builtin(mirror ofGetAddresswith optimizations removed)modified: src/emit/soroban/mod.rs
Added tests from pull 1921 + 2 extra boundary variables
modified: tests/soroban_testcases/mod.rs
new file: tests/soroban_testcases/block_number.rs
Compliant with
Pleas do let me know if anything seems out of place #