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/docs(CWPool): contract messages and basic readme #4792

Merged
merged 3 commits into from
May 26, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Mar 30, 2023

Closes: #XXX

What is the purpose of the change

This PR merges #4675 in smaller chunks.

The change here focuses on generating message boilerplate to interact between the chain and the contract.

Additionally, it adds basic READMEs. No state breaks in this PR.

Review suggestion: only skim proto folder and README.md. This is under 300 LOC, the rest is generated boilerplate.

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

@p0mvn p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Mar 30, 2023
@p0mvn p0mvn marked this pull request as ready for review March 30, 2023 03:39
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Mar 30, 2023
];
}

message SwapExactAmountOutSudoMsg {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're gonna launch the contract first and pools chain side later, then the messages need to be execute, not sudo (as only the chain can send sudo messages).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Let me cut an issue to discuss with Boss in a future iteration

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, this should still be a sudo message, and the contract refactored and upgraded.

We should be customizing the contract to make the chain code as secure and general as possible instead of adjusting chain code to be transmuter-specific

Copy link
Member 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.

I still think it makes sense for this to be an exec. Is there a reason we don't want people using the pools directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagged you here with the reasoning: #4810 (comment)

Even if we decide to change this in the future, let's not block this PR on this request, please. The reason is that everything else in the spike #4675, including the test contract, depends on these messages being sudo. As a result, it would be blocked on Boss refactoring the test contract.

I will make sure to use #4797 as a tracker to address this based on the conclusions from discussions we are having here: #4810 (comment)

Let me know if that works


// CalcOutAmtGivenIn
func NewCalcOutAmtGivenInRequest(tokenIn sdk.Coin, tokenOutDenom string, swapFee sdk.Dec) CalcOutAmtGivenInRequest {
return CalcOutAmtGivenInRequest{
Copy link
Contributor

Choose a reason for hiding this comment

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

Just our of curiosity, these functions should always return the the same as the inputs for transmuter, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand the question - but these functions are just meant to reduce boilerpate from callers. Instead of hardcoding the entire struct request every time, we can call this helper constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that makes sense. My understanding of transmutter is that it just swaps tokens 1:1. So I guess these functions don't have anything to calculate: send 1 axlusdc, get 1 nobleusdc.

Copy link
Member Author

Choose a reason for hiding this comment

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

These abstractions are enforced by PoolI and PoolModuleI interfaces from x/poolmanager. We use them in x/gamm and x/concentrated-liquidity as well.

Calc in this context means that this is the same operation as swap but does not persist any updates to state (is non-mutative)

@nicolaslara
Copy link
Contributor

With the exception of the sudo messages and the proto checks failing, this lgtm

@p0mvn p0mvn requested a review from nicolaslara March 30, 2023 16:35
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Apr 15, 2023
@github-actions github-actions bot closed this Apr 22, 2023
@p0mvn p0mvn reopened this May 26, 2023
@@ -0,0 +1,39 @@
# CosmWasm Pool Module
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: @iboss-ptk I'm down to eventually overwrite/merge this with yours #4810, and can help with resolving conflicts

@p0mvn
Copy link
Member Author

p0mvn commented May 26, 2023

Thanks! Merging. Will refresh #4675 and continue working there

@p0mvn p0mvn merged commit e5a2047 into main May 26, 2023
@p0mvn p0mvn deleted the roman/cw-pool-msg-protos branch May 26, 2023 13:13
pysel pushed a commit that referenced this pull request Jun 6, 2023
* feat/docs(CWPool): contract messages and basic readme

* proto gen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:no-changelog C:docs Improvements or additions to documentation Stale V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants