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

Test/solana #2139

Merged
merged 18 commits into from
Feb 7, 2024
Merged

Test/solana #2139

merged 18 commits into from
Feb 7, 2024

Conversation

0xArdi
Copy link
Collaborator

@0xArdi 0xArdi commented Jan 10, 2024

Proposed changes

This PR:

  • Adds a simple agent + skill to test solana interactions.
  • Fixes multiple small issues discovered during testing.

Types of changes

What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an x in the box that applies

  • Non-breaking fix (non-breaking change which fixes an issue)
  • Breaking fix (breaking change which fixes an issue)
  • Non-breaking feature (non-breaking change which adds functionality)
  • Breaking feature (breaking change which adds functionality)
  • Refactor (non-breaking change which changes implementation)
  • Messy (mixture of the above - requires explanation!)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the main branch (left side). Also you should start your branch off our main.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have locally run services that could be impacted and they do not present failures derived from my changes
  • Public-facing documentation has been updated with the changes affected by this PR. Even if the provided contents are not in their final form, all significant information must be included.
  • Any backwards-incompatible/breaking change has been clearly documented in the upgrading document.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Comment on lines 64 to 67
def _decode_instructions(instructions: str) -> List[Any]:
"""Decode the instructions."""
instructions = json.loads(instructions)
return [json.loads(instruction) for instruction in instructions]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Adamantios suggests we should store this in a serialized form. It will make such rounds much simpler in the future. I agree with that suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

So I tried to address this here d2a88ad

The contract method was returning a serialised instruction string instead of a dictionary object, I updated the transfer ix method to return a dictionary to match the rest of the methods

Comment on lines 1597 to 1599
# the check whether its solana is temporary until we have issues in the plugin
# fixed https://github.com/valory-xyz/open-aea/pull/708#discussion_r1448366974
if tx_hash != tx_hash_backup and chain_id != SOLANA_LEDGER_ID:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should not be the case anymore, that PR is merged @angrybayblade

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -1687,6 +1690,7 @@ def get_contract_api_response(
contract_address: Optional[str],
contract_id: str,
contract_callable: str,
ledger_id: Optional[str] = None,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

non-breaking

Comment on lines 64 to 67
def _decode_instructions(instructions: str) -> List[Any]:
"""Decode the instructions."""
instructions = json.loads(instructions)
return [json.loads(instruction) for instruction in instructions]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@angrybayblade angrybayblade marked this pull request as ready for review February 5, 2024 12:32
Comment on lines +192 to +194
# TOFIX - anchorpy has a conflict with tomte[tests]
package_dependencies.pop("open-aea-ledger-solana")
package_dependencies.pop("solders")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll create a tomte release with a fix

@angrybayblade angrybayblade force-pushed the test/solana branch 2 times, most recently from c013063 to 73cced5 Compare February 6, 2024 05:10
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (4cdb312) 94.21% compared to head (36c1572) 94.48%.
Report is 308 commits behind head on feat/solana.

Files Patch % Lines
packages/valory/connections/abci/connection.py 75.47% 13 Missing ⚠️
autonomy/chain/mint.py 88.04% 11 Missing ⚠️
autonomy/chain/service.py 95.40% 4 Missing ⚠️
autonomy/chain/base.py 77.77% 2 Missing ⚠️
...lory/skills/abstract_round_abci/behaviour_utils.py 77.77% 2 Missing ⚠️
autonomy/cli/helpers/chain.py 96.29% 1 Missing ⚠️
...es/valory/agents/solana_transfer_agent/__init__.py 0.00% 1 Missing ⚠️
...y/skills/transaction_settlement_abci/behaviours.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           feat/solana    #2139      +/-   ##
===============================================
+ Coverage        94.21%   94.48%   +0.26%     
===============================================
  Files              262      257       -5     
  Lines            15989    15917      -72     
===============================================
- Hits             15064    15039      -25     
+ Misses             925      878      -47     
Flag Coverage Δ
unittests 94.48% <92.75%> (+0.26%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator Author

@0xArdi 0xArdi left a comment

Choose a reason for hiding this comment

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

Changes look good @angrybayblade , I cannot approve as I am the PR creator.

@angrybayblade angrybayblade merged commit 080adbb into feat/solana Feb 7, 2024
21 of 22 checks passed
@DavidMinarsch DavidMinarsch deleted the test/solana branch March 13, 2024 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants