Skip to content
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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

feat: stacks #394

wants to merge 23 commits into from

Conversation

CyrusVorwald
Copy link
Contributor

Resolves #373, #374, #375, #376, #377, #378, #379

@CyrusVorwald CyrusVorwald self-assigned this Oct 12, 2024
@CyrusVorwald CyrusVorwald added the enhancement New feature or request label Oct 12, 2024
Copy link

codecov bot commented Oct 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.22%. Comparing base (9c03a0b) to head (5f1271e).
Report is 89 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             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              
Flag Coverage Δ
solidity 87.55% <ø> (-0.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

@CyrusVorwald CyrusVorwald marked this pull request as ready for review October 12, 2024 06:45
Copy link
Collaborator

@AntonAndell AntonAndell left a 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
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

message types?

Copy link
Contributor Author

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

Copy link
Collaborator

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
Copy link
Collaborator

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))
Copy link
Collaborator

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?

Copy link
Contributor Author

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"?

Comment on lines 539 to 541
(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 })
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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)
Copy link
Collaborator

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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)))
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define Network Address Representation in Clarity
2 participants