-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: stacks #394
base: main
Are you sure you want to change the base?
feat: stacks #394
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
============================================
- Coverage 89.47% 89.22% -0.26%
Complexity 77 77
============================================
Files 42 42
Lines 2698 2830 +132
Branches 37 37
============================================
+ Hits 2414 2525 +111
- Misses 267 288 +21
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks close to done. Seems you solved upgradability and such in a pretty cool way
ERR_NO_DEFAULT_CONNECTION) | ||
) | ||
|
||
(define-public (send-call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think message types have not been done so far right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it follows the same message types as contracts/javascore/xcall-lib/src/main/java/foundation/icon/xcall/CSMessage.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | ||
(asserts! (is-some connection-result) ERR_INVALID_NETWORK_ADDRESS) | ||
(emit-call-message-sent-event tx-sender to next-sn) | ||
(map-set outgoing-messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only set if there is a rollback
(receiver-principal (contract-of receiver)) | ||
(from (unwrap! (as-max-len? (unwrap! (get-network-address) ERR_NOT_INITIALIZED) u128) ERR_ADDRESS_TO_PRINCIPAL_FAILED)) | ||
(protocols (default-to (list) (get sources message))) | ||
(rollback (unwrap! (get rollback message) ERR_NO_ROLLBACK_DATA)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this set directly on send_call and thus this could be executed directly after sending no matter the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the question. What is "this"?
(try! (contract-call? receiver handle-call-message from data protocols common)) | ||
(emit-call-executed-event req-id CS_MESSAGE_RESULT_SUCCESS "") | ||
(map-delete incoming-messages { req-id: req-id }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this fail or how does this behave? This is where if there is rollback is checks if it fails and continues. If persistent it reverts the whole tx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the contract call fails, it throws an error and exits the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But a rollback message does not fail if it fails. It then still removes the message and notifies the connection that it should respond with a failure. Seems no response has been built here
(try! (contract-call? receiver handle-call-message from data protocols common)) | ||
(emit-call-executed-event req-id CS_MESSAGE_RESULT_SUCCESS "") | ||
(map-delete incoming-messages { req-id: req-id }) | ||
(ok true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing logic to rollback in case of failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is it supposed to do if the contract call fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respond with a failure message so that it can rollback
((fee (unwrap! (get-fee to (> sn 0)) ERR_INVALID_FEE))) | ||
(asserts! (>= (stx-get-balance tx-sender) fee) ERR_INVALID_FEE) | ||
(var-set conn-sn (+ (var-get conn-sn) 1)) | ||
(as-contract (unwrap-panic (contract-call? .xcall-proxy handle-message to msg implementation))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I think this was when I was initially trying to figure out how to call the proxy
Resolves #373, #374, #375, #376, #377, #378, #379